Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a11y-use-next-tooltip rule #103
Add a11y-use-next-tooltip rule #103
Changes from 1 commit
ded3726
7d7c94b
21a0497
27c7e24
613795b
cfca999
924cdc4
b584b8a
ec3ca30
4f7c1d1
4435db4
be1595e
aaf53d2
773698b
d651470
0b3f2f7
1d187b9
5002f33
1bfbc8a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 9 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 10 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 11 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
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.
Not sure if we need this line of code too?
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.
Probably not 😆 I was just adding examples to replicate real life cases but if it is confusing or you think it doesn't seem very related, I can totally remove it.
Check failure on line 20 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 25 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 29 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 30 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 31 in src/rules/__tests__/use-next-tooltip.test.js
GitHub Actions / lint
Check failure on line 9 in src/rules/use-next-tooltip.js
GitHub Actions / lint
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.
I didn't add a fix option that updates the path from
@primer/react
to@primer/react/next
because most of the time, Tooltip component itself will require updates as well. Let me know if you have a thought though!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.
I believe its still ok to fix. Any lessening of manual work is welcome right?
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.
I think so! My only concern is that if we provide a fix to update the path, would it give an impression that "you are done and good to go"? Because after fixing the path, in many cases Tooltip will need an update too (i.e.
aria-label
→text
or addingtype="description"
etc)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.
ok cool 👍 we can add it if someone requests it
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.
Will the typescript compiler warn about those issues?
Bonus: Can aria-label → text also be fixed by the linter? 😇
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.
(We chatted about these questions in our call with Sid, writing here for visibility)
It does now :) With making the text prop required. primer/react#3925
😇 I'm working on it to see if it is feasible to update the path as well as aria-label > text in one rule.
If anyone has any idea, thoughts, I'd love to hear 🙏🏻
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 like it worked
Check failure on line 14 in src/rules/use-next-tooltip.js
GitHub Actions / lint