Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: better rules table (filter rules by extension, etc.) #7666

Merged
merged 21 commits into from Oct 18, 2023

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Sep 17, 2023

PR Checklist

Overview

  • This PR changes the docs in the way that Brad discusses in the linked issue.
  • I got rid of all of the existing sidebar categories, since they do not make sense in the context of having a sortable table.
  • I also created a "constants.ts" file to remove some existing DRY violations that I found in the repository.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @Zamiell!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Sep 17, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 49ec745
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/652fe5c840e8a000086cd26c
😎 Deploy Preview https://deploy-preview-7666--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 89 (🔴 down 6 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@Zamiell Zamiell marked this pull request as ready for review September 17, 2023 17:44
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments, what do you think?

packages/eslint-plugin/docs/rules/README.md Show resolved Hide resolved
packages/website/src/components/RulesTable/index.tsx Outdated Show resolved Hide resolved
packages/website/src/components/RulesTable/index.tsx Outdated Show resolved Hide resolved
packages/website/src/components/RulesTable/index.tsx Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
Josh-Cena
Josh-Cena previously approved these changes Oct 13, 2023
Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, but will wait on @JoshuaKGoldberg

packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
packages/website/src/components/RulesTable/index.tsx Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
@Zamiell Zamiell changed the title docs: better rules table (sort rules by extension, etc.) docs: better rules table (filter rules by extension, etc.) Oct 13, 2023
Josh-Cena
Josh-Cena previously approved these changes Oct 13, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! I'm mostly happy - just suggesting some streamlining to the docs. WDYT? ✨

packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
packages/website/sidebars/sidebar.rules.js Show resolved Hide resolved
packages/eslint-plugin/docs/rules/README.md Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Oct 15, 2023
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@Zamiell
Copy link
Contributor Author

Zamiell commented Oct 15, 2023

Looking great! I'm mostly happy - just suggesting some streamlining to the docs. WDYT?

All the feedback should be committed now.

@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 18, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 thanks, this is looking great!


## Extension Rules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized https://typescript-eslint.io/rules/#extension-rules is probably the only canonical link to an explanation of extension rules out there. So we probably don't want to get rid of the heading. But I've pestered you so much on this PR 😅 I'll just add it in, and if you @Josh-Cena / @Zamiell disagree we can always revert before the release on Monday.

Copy link
Contributor Author

@Zamiell Zamiell Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need a separate heading though? it is fairly self-explanatory what an extension rule is.
like, for example, consider class-methods-use-this.
if you go to the page for it, it says:

This rule extends the base eslint/class-methods-use-this rule. It adds support for ignoring override methods or methods on classes that implement an interface.

After reading this, i am not left wondering, "I have more questions about what an extension rule is, I wonder if there is a more formal definition out there in a dedicated section?"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think folks need somewhere to hard-link to. It's similar to ESLint's issue for fixers vs suggestions: even if they're fairly straightforward for many, some folks coming in fresh have a hard time with them. I personally had a hard time both with fixers-vs-suggestions and what extension rules are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh and - pretty soon I think we'll be able to trim down the metadata list down to one sentence per item!

...which just leaves the deeper explanation on extension rules.

@JoshuaKGoldberg JoshuaKGoldberg merged commit ee4fe89 into typescript-eslint:main Oct 18, 2023
46 checks passed
@JoshuaKGoldberg
Copy link
Member

Whew, what a PR! Thanks again @Zamiell for iterating it on it with us. Shoutout @rubiesonthesky too!

@Josh-Cena I really liked your feedback and hope I didn't clobber over it in my eagerness to get the PR merged. :3

@Zamiell Zamiell deleted the docs-rule-formatting branch October 18, 2023 14:42
@Zamiell
Copy link
Contributor Author

Zamiell commented Oct 18, 2023

do we have a plan for mouseover-functionality-indication?

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Oct 18, 2023

I personally have none. If you want to file & tackle that as a followup issue it could be nice. I'm also wondering if we'd want to rely on a more rich table library that has sorting, filtering, etc. built-in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Formatting / Deprecated Rules are not clearly delineated in the rule list
4 participants