-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Move ESLint rules specific to @wordpress/components to custom rules
#74611
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 all commits
fa8cbd8
00db3f9
9ad09de
bb26f3a
15fa461
558bdcb
0b2f736
c0c4073
1f595f7
908f110
37ff6ec
613b531
fb8e4c4
706dbf3
49fe558
8c2d627
7c6c699
1b2bb4e
7fb5623
08dda50
136012a
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 |
|---|---|---|
|
|
@@ -108,16 +108,6 @@ const restrictedSyntax = [ | |
| }, | ||
| ]; | ||
|
|
||
| /** `no-restricted-syntax` rules for components. */ | ||
| const restrictedSyntaxComponents = [ | ||
| { | ||
| selector: | ||
| 'JSXOpeningElement[name.name="Button"]:not(:has(JSXAttribute[name.name="accessibleWhenDisabled"])) JSXAttribute[name.name="disabled"]', | ||
| message: | ||
| '`disabled` used without the `accessibleWhenDisabled` prop. Disabling a control without maintaining focusability can cause accessibility issues, by hiding their presence from screen reader users, or preventing focus from returning to a trigger element. (Ignore this error if you truly mean to disable.)', | ||
| }, | ||
| ]; | ||
|
|
||
| module.exports = { | ||
| root: true, | ||
| extends: [ | ||
|
|
@@ -257,11 +247,8 @@ module.exports = { | |
| ], | ||
| excludedFiles: [ '**/*.native.js' ], | ||
| rules: { | ||
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| ...restrictedSyntax, | ||
| ...restrictedSyntaxComponents, | ||
| ], | ||
| 'no-restricted-syntax': [ 'error', ...restrictedSyntax ], | ||
| '@wordpress/components-no-unsafe-button-disabled': 'error', | ||
| }, | ||
| }, | ||
| { | ||
|
|
@@ -271,47 +258,9 @@ module.exports = { | |
| '**/*.@(native|ios|android).js', | ||
| ], | ||
| rules: { | ||
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| ...restrictedSyntax, | ||
| ...restrictedSyntaxComponents, | ||
| // Temporary rules until we're ready to officially default to the new size. | ||
| ...[ | ||
| 'BorderBoxControl', | ||
| 'BorderControl', | ||
| 'BoxControl', | ||
| 'Button', | ||
| 'ComboboxControl', | ||
| 'CustomSelectControl', | ||
|
|
||
| 'FontAppearanceControl', | ||
| 'FontFamilyControl', | ||
| 'FontSizePicker', | ||
| 'FormTokenField', | ||
| 'InputControl', | ||
| 'LetterSpacingControl', | ||
| 'LineHeightControl', | ||
| 'NumberControl', | ||
| 'RangeControl', | ||
| 'SelectControl', | ||
| 'TextControl', | ||
| 'ToggleGroupControl', | ||
| 'UnitControl', | ||
| ].map( ( componentName ) => ( { | ||
| // Falsy `__next40pxDefaultSize` without a non-default `size` prop. | ||
| selector: `JSXOpeningElement[name.name="${ componentName }"]:not(:has(JSXAttribute[name.name="__next40pxDefaultSize"][value.expression.value!=false])):not(:has(JSXAttribute[name.name="size"][value.value!="default"]))`, | ||
| message: | ||
| componentName + | ||
| ' should have the `__next40pxDefaultSize` prop when using the default size.', | ||
| } ) ), | ||
| { | ||
| // Falsy `__next40pxDefaultSize` without a `render` prop. | ||
| selector: | ||
| 'JSXOpeningElement[name.name="FormFileUpload"]:not(:has(JSXAttribute[name.name="__next40pxDefaultSize"][value.expression.value!=false])):not(:has(JSXAttribute[name.name="render"]))', | ||
| message: | ||
| 'FormFileUpload should have the `__next40pxDefaultSize` prop to opt-in to the new default size.', | ||
| }, | ||
| ], | ||
|
Comment on lines
-278
to
-314
Contributor
Author
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. This rule is now replaced by the new |
||
| 'no-restricted-syntax': [ 'error', ...restrictedSyntax ], | ||
| '@wordpress/components-no-unsafe-button-disabled': 'error', | ||
| '@wordpress/components-no-missing-40px-size-prop': 'error', | ||
| }, | ||
| }, | ||
| { | ||
|
|
@@ -443,7 +392,6 @@ module.exports = { | |
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| ...restrictedSyntax, | ||
| ...restrictedSyntaxComponents, | ||
| { | ||
| selector: | ||
| ':matches(Literal[value=/--wp-admin-theme-/],TemplateElement[value.cooked=/--wp-admin-theme-/])', | ||
|
|
@@ -460,6 +408,22 @@ module.exports = { | |
| ], | ||
| }, | ||
| }, | ||
| { | ||
| // Override the @wordpress/components-* rules by adding the | ||
| // `checkLocalImports` flag, which adds the linting also to relative | ||
| // imports. | ||
| files: [ 'packages/components/src/**' ], | ||
| rules: { | ||
| '@wordpress/components-no-unsafe-button-disabled': [ | ||
| 'error', | ||
| { checkLocalImports: true }, | ||
| ], | ||
| // '@wordpress/components-no-missing-40px-size-prop': [ | ||
| // 'error', | ||
| // { checkLocalImports: true }, | ||
| // ], | ||
|
Comment on lines
+421
to
+424
Contributor
Author
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. With the current ESLint configuration, we were not checking for the I'll work on a follow-up PR where I enable this rule and fix all errors.
Contributor
Author
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. Follow-up: #74622
ciampo marked this conversation as resolved.
ciampo marked this conversation as resolved.
|
||
| }, | ||
| }, | ||
| { | ||
| files: [ 'packages/components/src/**' ], | ||
| excludedFiles: [ 'packages/components/src/**/@(test|stories)/**' ], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,8 @@ function ButtonBlockAppender( | |
| ); | ||
|
|
||
| return ( | ||
| // Disable reason: There shouldn't be a case where this button is disabled but not visually hidden. | ||
| // eslint-disable-next-line @wordpress/components-no-unsafe-button-disabled | ||
|
Comment on lines
+57
to
+58
Contributor
Author
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. The new rule errors at the JSX tag level instead of the prop level. Another improvement over the old rule, is that disabling the rule would also disable all other |
||
| <Button | ||
| __next40pxDefaultSize | ||
| ref={ ref } | ||
|
|
@@ -66,8 +68,6 @@ function ButtonBlockAppender( | |
| onClick={ onToggle } | ||
| aria-haspopup={ isToggleButton ? 'true' : undefined } | ||
| aria-expanded={ isToggleButton ? isOpen : undefined } | ||
| // Disable reason: There shouldn't be a case where this button is disabled but not visually hidden. | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| disabled={ disabled } | ||
| label={ label } | ||
| showTooltip | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ function FontSizePicker( props ) { | |
| { ...props } | ||
| fontSizes={ fontSizes } | ||
| disableCustomFontSizes={ ! customFontSize } | ||
| __next40pxDefaultSize | ||
|
Contributor
Author
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. The new rules are able to track renamed imports 🚀 |
||
| /> | ||
| ); | ||
| } | ||
|
|
||
|
Contributor
Author
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. Changes in this file are a direct consequence of https://github.com/WordPress/gutenberg/pull/74611/changes#r2690292410. In a follow-up, I will either add the |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # Disallow missing `__next40pxDefaultSize` prop (components-no-missing-40px-size-prop) | ||
|
|
||
| Enforces that specific components from `@wordpress/components` include the `__next40pxDefaultSize` prop to opt-in to the new 40px default size. | ||
|
|
||
| This is a temporary rule to help migrate components to the new default size. Once the grace period is over and all components default to the new 40px size, this rule can be removed. | ||
|
|
||
| ## Rule details | ||
|
|
||
| The following components are checked by this rule: | ||
|
|
||
| - BorderBoxControl | ||
| - BorderControl | ||
| - BoxControl | ||
| - Button | ||
| - ComboboxControl | ||
| - CustomSelectControl | ||
| - FontAppearanceControl | ||
| - FontFamilyControl | ||
| - FontSizePicker | ||
| - FormFileUpload (special case - see below) | ||
| - FormTokenField | ||
| - InputControl | ||
| - LetterSpacingControl | ||
| - LineHeightControl | ||
| - NumberControl | ||
| - RangeControl | ||
| - SelectControl | ||
| - TextControl | ||
| - ToggleGroupControl | ||
| - UnitControl | ||
|
|
||
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```jsx | ||
| import { Button, InputControl } from '@wordpress/components'; | ||
|
|
||
| <Button>Click me</Button> | ||
| <InputControl value={value} onChange={onChange} /> | ||
| <Button __next40pxDefaultSize={false}>Click me</Button> | ||
| <Button size="default">Click me</Button> | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```jsx | ||
| import { Button, InputControl } from '@wordpress/components'; | ||
|
|
||
| <Button __next40pxDefaultSize>Click me</Button> | ||
| <Button __next40pxDefaultSize={true}>Click me</Button> | ||
| <InputControl __next40pxDefaultSize value={value} onChange={onChange} /> | ||
| <Button size="small">Click me</Button> | ||
| <Button size="compact">Click me</Button> | ||
| ``` | ||
|
|
||
| ## FormFileUpload special case | ||
|
|
||
| `FormFileUpload` can use either the `__next40pxDefaultSize` prop or the `render` prop to be considered valid: | ||
|
|
||
| ```jsx | ||
| import { FormFileUpload } from '@wordpress/components'; | ||
|
|
||
| // Both are valid: | ||
| <FormFileUpload __next40pxDefaultSize /> | ||
| <FormFileUpload render={({ open }) => <button onClick={open}>Upload</button>} /> | ||
| ``` | ||
|
|
||
| ## Options | ||
|
|
||
| ### checkLocalImports | ||
|
|
||
| When set to `true`, the rule also checks components imported from relative paths. This is useful inside the `@wordpress/components` package itself, where components are imported via relative paths instead of `@wordpress/components`. | ||
|
|
||
| ```json | ||
| { | ||
| "@wordpress/components-no-missing-40px-size-prop": [ | ||
| "error", | ||
| { "checkLocalImports": true } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| ## Important notes | ||
|
|
||
| - By default, this rule only applies to components imported from `@wordpress/components`. | ||
| - Components from other packages (like `@wordpress/ui`) or locally defined components with the same name are not affected. | ||
| - Aliased imports (e.g., `import { Button as WPButton }`) are correctly tracked. | ||
| - Components with a non-default `size` prop (e.g., `size="small"`, `size="compact"`) are exempt from this rule. | ||
|
mirka marked this conversation as resolved.
|
||
| - Use the `checkLocalImports` option when linting inside the `@wordpress/components` package. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # Disallow using disabled on Button without accessibleWhenDisabled (components-no-unsafe-button-disabled) | ||
|
|
||
| Disabling a `Button` component without maintaining focusability can cause accessibility issues by hiding its presence from screen reader users, or preventing focus from returning to a trigger element. | ||
|
|
||
| This rule enforces that `Button` components from `@wordpress/components` include the `accessibleWhenDisabled` prop when the `disabled` prop is set. | ||
|
|
||
| ## Rule details | ||
|
|
||
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```jsx | ||
| import { Button } from '@wordpress/components'; | ||
|
|
||
| <Button disabled>Click me</Button> | ||
| <Button disabled={true}>Click me</Button> | ||
| <Button disabled={someVar}>Click me</Button> | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```jsx | ||
| import { Button } from '@wordpress/components'; | ||
|
|
||
| <Button disabled accessibleWhenDisabled>Click me</Button> | ||
| <Button disabled accessibleWhenDisabled={true}>Click me</Button> | ||
| <Button disabled accessibleWhenDisabled={false}>Click me</Button> | ||
| <Button disabled accessibleWhenDisabled={someVar}>Click me</Button> | ||
| <Button disabled={false}>Click me</Button> | ||
| <Button onClick={handleClick}>Click me</Button> | ||
| ``` | ||
|
|
||
| ## Options | ||
|
|
||
| ### checkLocalImports | ||
|
|
||
| When set to `true`, the rule also checks `Button` components imported from relative paths. This is useful inside the `@wordpress/components` package itself, where components are imported via relative paths instead of `@wordpress/components`. | ||
|
|
||
| ```json | ||
| { | ||
| "@wordpress/components-no-unsafe-button-disabled": [ | ||
| "error", | ||
| { "checkLocalImports": true } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| ## Important notes | ||
|
|
||
| - By default, this rule only applies to `Button` components imported from `@wordpress/components`. | ||
| - `Button` components from other packages (like `@wordpress/ui`) or locally defined components with the same name are not affected. | ||
| - Aliased imports (e.g., `import { Button as WPButton }`) are correctly tracked. | ||
| - Use the `checkLocalImports` option when linting inside the `@wordpress/components` package. |
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.
restrictedSyntaxComponentsis effectively replaced by the new@wordpress/components-no-unsafe-button-disabledrule