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

Add a11y-use-next-tooltip rule #103

Merged
merged 19 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions docs/rules/use-next-tooltip.md
broccolinisoup marked this conversation as resolved.
Show resolved Hide resolved
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'
```
32 changes: 32 additions & 0 deletions src/rules/__tests__/use-next-tooltip.test.js
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

Check failure on line 9 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
}

Check failure on line 10 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
}

Check failure on line 11 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
})

ruleTester.run('use-next-tooltip', rule, {
valid: [
`import {Tooltip} from '@primer/react/next'`,
`import {UnderlineNav, Button} from '@primer/react';
Copy link
Contributor

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?

Copy link
Member Author

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.

import {Tooltip} from '@primer/react/next';`,
`import {UnderlineNav, Button} from '@primer/react';
import {Tooltip, SelectPanel} from '@primer/react/next';`

Check failure on line 20 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
],
invalid: [
{
code: `import {Tooltip} from '@primer/react'`,
errors: [{messageId: 'useNextTooltip'}]

Check failure on line 25 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
},
{
code: `import {Tooltip, Button} from '@primer/react'`,
errors: [{messageId: 'useNextTooltip'}]

Check failure on line 29 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
}

Check failure on line 30 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
]

Check failure on line 31 in src/rules/__tests__/use-next-tooltip.test.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
})
36 changes: 36 additions & 0 deletions src/rules/use-next-tooltip.js
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

Check failure on line 9 in src/rules/use-next-tooltip.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
},
fixable: null,
Copy link
Member Author

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!

Copy link
Contributor

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?

Copy link
Member Author

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-labeltext or adding type="description" etc)

Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

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

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? 😇

Copy link
Member Author

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)

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 🙏🏻

Copy link
Member Author

Choose a reason for hiding this comment

The 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'

Check failure on line 14 in src/rules/use-next-tooltip.js

View workflow job for this annotation

GitHub Actions / lint

Insert `,`
}
},
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'
})
}
}
}
}
Loading