SelectControl: Add lint rule for 40px size prop usage #64486
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
28 tasks
|
Size Change: +300 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
mirka
commented
Aug 14, 2024
| @@ -71,9 +71,18 @@ The current font family value. | |||
|
|
|||
| The rest of the props are passed down to the underlying `<SelectControl />` instance. | |||
|
|
|||
| #### `__next40pxDefaultSize` | |||
There was a problem hiding this comment.
By the way this prop is not yet documented in any of the @wordpress/components READMEs at the moment, just in Storybook. I'll do a PR soon to add them all.
| @@ -50,6 +52,7 @@ export default function FontFamilyControl( { | |||
|
|
|||
| return ( | |||
| <SelectControl | |||
| __next40pxDefaultSize={ __next40pxDefaultSize } | |||
There was a problem hiding this comment.
This was already passed through via rest props, but making it explicit here.
| @@ -40,6 +40,7 @@ const renderTestDefaultControlComponent = ( labelComponent, device ) => { | |||
| return ( | |||
| <> | |||
| <SelectControl | |||
| __next40pxDefaultSize | |||
There was a problem hiding this comment.
Doesn't matter because this is a test.
| @@ -51,6 +51,7 @@ export default function ArchivesEdit( { attributes, setAttributes } ) { | |||
| } | |||
| /> | |||
| <SelectControl | |||
| __next40pxDefaultSize | |||
| @@ -174,6 +174,7 @@ function AudioEdit( { | |||
| checked={ loop } | |||
| /> | |||
| <SelectControl | |||
| __next40pxDefaultSize | |||
| @@ -73,6 +73,7 @@ export default function FileBlockInspector( { | |||
| ) } | |||
| <PanelBody title={ __( 'Settings' ) }> | |||
| <SelectControl | |||
| __next40pxDefaultSize | |||
| @@ -112,6 +112,7 @@ function PostAuthorEdit( { | |||
| /> | |||
| ) ) || ( | |||
| <SelectControl | |||
| __next40pxDefaultSize | |||
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ciampo
approved these changes
Aug 15, 2024
Comment on lines
201
to
204
| Disable the audio tag if the block is not selected | ||
| so the user clicking on it won't play the | ||
| file or change the position slider when the controls are enabled. | ||
| */ } |
There was a problem hiding this comment.
is this mix of spaces and tabs in the file expected?
There was a problem hiding this comment.
Weird, I reformatted it and it changed the spaces back to tabs, but it still doesn't match the original. Not sure why it isn't deterministic 🤷 892c9c1
| @@ -130,6 +130,8 @@ const Edit = ( { attributes, setAttributes, clientId } ) => { | |||
| { submissionMethod !== 'email' && ( | |||
| <InspectorControls group="advanced"> | |||
| <SelectControl | |||
| // TODO: Switch to `true` (40px size) if possible | |||
| __next40pxDefaultSize={ false } | |||
There was a problem hiding this comment.
(excuses if this has already been asked) Do we have a place where we track all the occurrences that need updating and are exception to the lint rule?
There was a problem hiding this comment.
I'm assuming you're asking about discoverability. A few ways:
- I'm leaving a separate checklist item in Components: Add eslint rules to prevent usage without
__next40pxDefaultSizeprop #63871 so we know there are remaining violations. - We can search the TODO comment string.
- We can move this component to the stricter lint rule in the eslintrc, which doesn't allow
__next40pxDefaultSize={ false }. That would surface all the remaining violations.
|
Another one down 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #63871
What?
Add a lint rule to prevent people from introducing new usages of SelectControl that do not adhere to the new default size, while adding a TODO note on existing violations.
I also went ahead and fixed the most straightforward ones.
Why?
Some components have a large number of existing violations, and it would take some time to safely switch them all. We can more efficiently move everything to the new 40px sizes if we immediately start preventing new violations from entering the codebase, as well as add TODO comments on the existing violations so people who come across that code are more likely to fix it on the spot.
How?
I codemod'ed all existing violations to:
Testing Instructions
The lint error should trigger if you add a SelectControl with no
__next40pxDefaultSizeorsizeprop.