Skip to content

Add a rule that warn against removing unsafeDisableTooltip prop from IconButton #185

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

Merged
merged 9 commits into from
Jun 13, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/slow-numbers-invite.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-primer-react": minor
---

Add a rule that warns against removing `unsafeDisableTooltip` prop
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,4 @@ ESLint rules for Primer React
- [a11y-tooltip-interactive-trigger](https://github.com/primer/eslint-plugin-primer-react/blob/main/docs/rules/a11y-tooltip-interactive-trigger.md)
- [a11y-explicit-heading](https://github.com/primer/eslint-plugin-primer-react/blob/main/docs/rules/a11y-explicit-heading.md)
- [a11y-link-in-text-block](https://github.com/primer/eslint-plugin-primer-react/blob/main/docs/rules/a11y-link-in-text-block.md)
- [a11y-remove-disable-tooltip](https://github.com/primer/eslint-plugin-primer-react/blob/main/docs/rules/a11y-remove-disable-tooltip.md)
25 changes: 25 additions & 0 deletions docs/rules/a11y-remove-disable-tooltip.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
## Rule Details

This rule enforces to remove the `unsafeDisableTooltip` from `IconButton` component so that they have a tooltip by default. `unsafeDisableTooltip` prop is created for an incremental migration and should be removed once all icon buttons have a tooltip.

👎 Examples of **incorrect** code for this rule:

```jsx
import {IconButton} from '@primer/react'

const App = () => (
<IconButton icon={SearchIcon} aria-label="Search" unsafeDisableTooltip />
// OR
<IconButton icon={SearchIcon} aria-label="Search" unsafeDisableTooltip={true} />
// OR
<IconButton icon={SearchIcon} aria-label="Search" unsafeDisableTooltip={false} /> // This is incorrect because it should be removed
)
```

👍 Examples of **correct** code for this rule:

```jsx
import {IconButton} from '@primer/react'

const App = () => <IconButton icon={SearchIcon} aria-label="Search" />
```
1 change: 1 addition & 0 deletions src/configs/recommended.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ module.exports = {
'primer-react/new-color-css-vars': 'error',
'primer-react/a11y-explicit-heading': 'error',
'primer-react/no-deprecated-props': 'warn',
'primer-react/a11y-remove-disable-tooltip': 'error',
},
settings: {
github: {
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module.exports = {
'a11y-explicit-heading': require('./rules/a11y-explicit-heading'),
'no-deprecated-props': require('./rules/no-deprecated-props'),
'a11y-link-in-text-block': require('./rules/a11y-link-in-text-block'),
'a11y-remove-disable-tooltip': require('./rules/a11y-remove-disable-tooltip'),
},
configs: {
recommended: require('./configs/recommended'),
Expand Down
50 changes: 50 additions & 0 deletions src/rules/__tests__/a11y-remove-disable-tooltip.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict'

const {RuleTester} = require('eslint')
const rule = require('../a11y-remove-disable-tooltip')

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 'latest',
sourceType: 'module',
ecmaFeatures: {
jsx: true,
},
},
})

ruleTester.run('a11y-remove-disable-tooltip', rule, {
valid: [
`import {IconButton} from '@primer/react';
<IconButton icon={SearchIcon} aria-label="Search" />`,
],
invalid: [
{
code: `<IconButton icon={SearchIcon} aria-label="Search" unsafeDisableTooltip />`,
output: `<IconButton icon={SearchIcon} aria-label="Search" />`,
errors: [
{
messageId: 'removeDisableTooltipProp',
},
],
},
{
code: `<IconButton icon={SearchIcon} aria-label="Search" unsafeDisableTooltip={true} />`,
output: `<IconButton icon={SearchIcon} aria-label="Search" />`,
errors: [
{
messageId: 'removeDisableTooltipProp',
},
],
},
{
code: `<IconButton icon={SearchIcon} aria-label="Search" unsafeDisableTooltip={false} />`,
output: `<IconButton icon={SearchIcon} aria-label="Search" />`,
errors: [
{
messageId: 'removeDisableTooltipProp',
},
],
},
],
})
Copy link
Member

Choose a reason for hiding this comment

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

(After we change the default value of unsafeDisableTooltip), Do you have a preference for unsafeDisableTooltip={false} vs not having it all? Should we add that to the linter?

Copy link
Member

Choose a reason for hiding this comment

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

To add to this, is the plan to deprecate the prop in the future once we know unsafeDisableTooltip={true} isn't being used anywhere in dotcom (or at least a very low amount of usage)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a preference for unsafeDisableTooltip={false} vs not having it all? Should we add that to the linter?

I really like the idea of adding it to the linter! I always imagined that we would remove the prop instead of unsafeDisableTooltip={false} - I'll add it to the rule.

is the plan to deprecate the prop in the future once we know unsafeDisableTooltip={true} isn't being used anywhere in dotcom (or at least a very low amount of usage)?

This is the hope and the plan 🙌🏻 - however there might be some rare cases that we need to hide the tooltip and I guess we are yet to discover these cases as product teams test and remove unsafeDisableTooltip={true}. If we end up having icon buttons that we certainly don't need tooltips for whatever reason, we might choose to keep the prop.

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'll add it to the rule.

Actually, I just realised that the rule checks unsafeDisableTooltip={false} as well. Or in better words, it doesn't check the value of the prop 😆 I think this is fine because type safety will handle the value and we just need to worry about if the prop exists or not. I added a test case and into the docs though. Let me know if you have any concern 🙌🏻

47 changes: 47 additions & 0 deletions src/rules/a11y-remove-disable-tooltip.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict'
const {getJSXOpeningElementAttribute} = require('../utils/get-jsx-opening-element-attribute')
const {getJSXOpeningElementName} = require('../utils/get-jsx-opening-element-name')

/**
* @type {import('eslint').Rule.RuleModule}
*/
module.exports = {
meta: {
type: 'error',
docs: {
description:
'Icon buttons should have tooltip by default. Please remove `unsafeDisableTooltip` prop from `IconButton` component to enable the tooltip and help making icon button more accessible.',
recommended: true,
},
fixable: 'code',
schema: [],
messages: {
removeDisableTooltipProp:
'Please remove `unsafeDisableTooltip` prop from `IconButton` component to enable the tooltip and help make icon button more accessible.',
},
},
create(context) {
return {
JSXOpeningElement(node) {
const openingElName = getJSXOpeningElementName(node)
if (openingElName !== 'IconButton') {
return
}
const unsafeDisableTooltip = getJSXOpeningElementAttribute(node, 'unsafeDisableTooltip')
if (unsafeDisableTooltip !== undefined) {
context.report({
node,
messageId: 'removeDisableTooltipProp',
fix(fixer) {
const start = unsafeDisableTooltip.range[0]
const end = unsafeDisableTooltip.range[1]
return [
fixer.removeRange([start - 1, end]), // remove the space before unsafeDisableTooltip as well
]
},
})
}
},
}
},
}
Loading