-
Notifications
You must be signed in to change notification settings - Fork 10
Add a11y-use-next-tooltip rule #103
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
Changes from 2 commits
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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# Recommends to use the `next` tooltip instead of the current stable one. | ||
|
||
## Rule details | ||
|
||
This rule recommends using the tooltip that is imported from `@primer/react/next` instead of the main entrypoint. The components that are exported from `@primer/react/next` are recommended over the main entrypoint ones because `next` components are reviewed and remediated for accessibility issues. | ||
👎 Examples of **incorrect** code for this rule: | ||
|
||
```tsx | ||
import {Tooltip} from '@primer/react' | ||
``` | ||
|
||
👍 Examples of **correct** code for this rule: | ||
|
||
```tsx | ||
import {Tooltip} from '@primer/react/next' | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
const rule = require('../use-next-tooltip') | ||
const {RuleTester} = require('eslint') | ||
|
||
const ruleTester = new RuleTester({ | ||
parserOptions: { | ||
ecmaVersion: 'latest', | ||
sourceType: 'module', | ||
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
}, | ||
}) | ||
|
||
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. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
import {Tooltip} from '@primer/react/next';`, | ||
`import {UnderlineNav, Button} from '@primer/react'; | ||
import {Tooltip, SelectPanel} from '@primer/react/next';`, | ||
], | ||
invalid: [ | ||
{ | ||
code: `import {Tooltip} from '@primer/react'`, | ||
errors: [{messageId: 'useNextTooltip'}], | ||
}, | ||
{ | ||
code: `import {Tooltip, Button} from '@primer/react'`, | ||
errors: [{messageId: 'useNextTooltip'}], | ||
}, | ||
], | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
meta: { | ||
type: 'suggestion', | ||
docs: { | ||
description: 'recommends the use of @primer/react/next Tooltip component', | ||
category: 'Best Practices', | ||
recommended: true, | ||
}, | ||
fixable: null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't add a fix option that updates the path from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it worked |
||
schema: [], | ||
messages: { | ||
useNextTooltip: 'Please use @primer/react/next Tooltip component that has accessibility improvements', | ||
}, | ||
}, | ||
create(context) { | ||
return { | ||
ImportDeclaration(node) { | ||
if (node.source.value !== '@primer/react') { | ||
return | ||
} | ||
const hasTooltip = node.specifiers.some( | ||
specifier => specifier.imported && specifier.imported.name === 'Tooltip', | ||
) | ||
if (!hasTooltip) { | ||
return | ||
} | ||
context.report({ | ||
node, | ||
messageId: 'useNextTooltip', | ||
}) | ||
}, | ||
} | ||
}, | ||
} |
Uh oh!
There was an error while loading. Please reload this page.