-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
docs: autogenerate rules table on website #5116
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: autogenerate rules table on website #5116
Conversation
Thanks for the PR, @Josh-Cena! 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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
I lazily reused a lot of styles from https://docusaurus.io/showcase/. Do we want to use the same card view instead of table view? Also currently the filters look a bit raw. There are only two states: "include" and "don't care". Ideally each checkbox should be tri-state: "include", "don't care", and "exclude". But the UI/UX is a bit hard to design. |
Let's see—much easier to experiment now. |
OK the filter logic is hard to make... We have three possible states:
But if all filters are "neutral" about a rule (either because they have "neutral" state or because the rule has no attributes), should we include it? This means we either have to have an option "default to include/exclude", or we expand the states to much more states, because we have two cases: a rule with/without attribute X, each with three outcomes: immediately include, immediately exclude, and remain neutral. And after that comes the question of priority when one filter wants to include and the other wants to exclude. Filters are hard to build... The only alternative is to allow writing custom queries 😄 We may define priority based on the order of clicking, but that feels a bit unwieldy as well. Anyways, the UI is here; what UX we want is up for discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry... force of habit at meta where we stamp and let the author make changes and ship whenever.
what do you guys think about changing those tables to have css
as right now while filtering size of it can shift alot |
Do I look like I know any CSS... 😄 I'll admit I tried this, and set |
803ff11
to
a8c8567
Compare
a8c8567
to
29bea86
Compare
that's depends if we want to keep them vertically or horizontally if horizontally I think its actually better |
Pushed a layout with vertically arranged icons. I like it better |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, awesome stuff! Just requesting changes on the additional rule table removal as you suggested, and the accessibility touchups. Thanks @Josh-Cena 💖!
related issue #4366 |
"fixable" / "has suggestions" are not necessarily mutually exclusive. In fact I'm not even sure if it's a good idea to not show "recommended" rules as not included in "strict" (sorry for the triple negation). I'm also thinking about a toggle between "AND" and "OR" of these filters, as seen on our showcase: https://docusaurus.io/showcase |
A rule can only be in one of those two rulesets. Rules that are in strict are not in recommended, and vice versa. This is confusing and keeps coming up (why would you enable strict without recommended?) so maybe it'll change. But at least in version 5.X+ the two have no overlap. |
Ugh, I have made that realization at least twice during my course of working on this, but I keep forgetting... Thanks for reminding again... |
Re ☝️ , #5204 |
@Josh-Cena just checking, is this ready for re-review? No worries if you haven't had time 🙂 just don't want to keep you waiting! This is going to be very good for the docs push, so I'm happy to take it over the finishing line if you're swamped with other things. |
I think the only pending issue is the accessibility of the badges, right? (Unless you guys want some other UX for the filters?) I'm indeed a bit occupied with other stuff these weeks (relocating to the US this summer so getting visas and all paperwork sorted out.) Would appreciate it if you can help out. Thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ship this thing!
PR Checklist
Overview
Let me know if you think it's fine to remove the table in the package's README and simply point people to the website.