Add a11y-use-next-tooltip rule #103
Conversation
🦋 Changeset detectedLatest commit: 1bfbc8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
src/rules/use-next-tooltip.js
Outdated
| category: 'Best Practices', | ||
| recommended: true, | ||
| }, | ||
| fixable: null, |
There was a problem hiding this comment.
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.
I believe its still ok to fix. Any lessening of manual work is welcome right?
There was a problem hiding this comment.
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 adding type="description" etc)
There was a problem hiding this comment.
ok cool 👍 we can add it if someone requests it
There was a problem hiding this comment.
in many cases Tooltip will need an update too (i.e. aria-label → text or adding type="description" etc)
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.
(We chatted about these questions in our call with Sid, writing here for visibility)
Will the typescript compiler warn about those issues?
It does now :) With making the text prop required. primer/react#3925
Bonus: Can aria-label → text also be fixed by the linter? 😇
😇 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.
Looks like it worked
broccolinisoup
requested review from
a team,
mperrotti,
pksjce,
siddharthkp and
joshblack
| ruleTester.run('use-next-tooltip', rule, { | ||
| valid: [ | ||
| `import {Tooltip} from '@primer/react/next'`, | ||
| `import {UnderlineNav, Button} from '@primer/react'; |
There was a problem hiding this comment.
Not sure if we need this line of code too?
There was a problem hiding this comment.
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.
src/rules/use-next-tooltip.js
Outdated
| category: 'Best Practices', | ||
| recommended: true, | ||
| }, | ||
| fixable: null, |
There was a problem hiding this comment.
I believe its still ok to fix. Any lessening of manual work is welcome right?
broccolinisoup
changed the title
broccolinisoup
force-pushed
the
bs/rule-for-tooltip-v2
branch
from
3449de2
to
d651470
Compare
Closes #114
As a part of Tooltip adoption discussion, we decided to introduce an eslint rule that recommends switch over to the Tooltip v2 which will be released under the
primer/react/nextbundle based on the naming discussion.The rule also suggests fixes to update the import from
@primer/reactto@primer/react/nextas well as updating the prop name fromaria-labeltotext