UnitControl: Add lint rule for 40px size prop usage #64520
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
|
Size Change: +207 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
mirka
commented
Aug 14, 2024
| @@ -38,13 +38,20 @@ The current value of the letter spacing setting. | |||
|
|
|||
| A callback function invoked when the value is changed. | |||
|
|
|||
| ### `_unstableInputWidth` | |||
| ### `__unstableInputWidth` | |||
| @@ -35,6 +37,7 @@ export default function LetterSpacingControl( { | |||
| } ); | |||
| return ( | |||
| <UnitControl | |||
| __next40pxDefaultSize={ __next40pxDefaultSize } | |||
There was a problem hiding this comment.
This was already passed through via otherProps, but making it explicit here.
| @@ -134,8 +134,7 @@ const DimensionControls = ( { | |||
| panelId={ clientId } | |||
| > | |||
| <SelectControl | |||
| // TODO: Switch to `true` (40px size) if possible | |||
| __next40pxDefaultSize={ false } | |||
| __next40pxDefaultSize | |||
There was a problem hiding this comment.
| Before | After |
|---|---|
 |
 |
Incorrect column gap will be addressed separately at #64488
| label={ __( 'Minimum height of cover' ) } | ||
| id={ inputId } | ||
| isResetValueOnUnitChange | ||
| min={ min } | ||
| onChange={ handleOnChange } | ||
| onUnitChange={ onUnitChange } | ||
| __unstableInputWidth="80px" | ||
| __unstableInputWidth="116px" |
There was a problem hiding this comment.
| Before | After |
|---|---|
 |
 |
This custom styling that allows the label to expand wider than the control is... not great. I agree that the visual UI looks the best this way in this specific instance, but I struggle to think of a universally nice way to capture this pattern. The custom styling is applied from an English-centric eye, so it's likely that we're not doing this for UnitControls that happen to have a label that doesn't overflow in English. And what if we ever change the font size or styling of the labels, and some labels that previously fit in a single column start to overflow?
Would it be worth making this label-expanding the default behavior for "single-column" controls that have enough space for the label to expand (not sure if even feasible)?
Other more simple guidelines to maintain consistency would be:
-
Don't do single-column when there is nothing in the second column
-
Don't do the label expanding when single-column
Thoughts? @WordPress/gutenberg-design
There was a problem hiding this comment.
I personally think "Min. height" would be okay, though this question has come up in the past and suggested that "Minimum height" is better.
But can it be a full-row, with input on the left, and slider on the right? This would only work when minimum height is the only control, where for example "Width" and "Height" make a good two-column pair.
There was a problem hiding this comment.
Ok, I think I can do "Minimum height" for this one. I guess the universal guideline remains that these labels should ideally fit within a single column, and no cheating just because it happens to overflow in English 😂
But can it be a full-row, with input on the left, and slider on the right?
This has come up before (#60633), but there's some disagreement as to whether sliders are appropriate for inputs without a clear upper bound, as is in this case.
There was a problem hiding this comment.
This is something that should be solved universally, and not on a case-by-case basis, especially since we can't predict the length of those labels when they get translated to other languages:
- we should keep our strings as short and clear as possible;
- we should have a universal solution for when these input label don't fit in their space — could we truncate + ellipse them?
There was a problem hiding this comment.
They are truncated + ellipsized by default. It's just that this case was initially "cheating" by expanding only the label into two columns.
There was a problem hiding this comment.
I see — IMO it would be probably better to remove the "cheat", if possible
There was a problem hiding this comment.
Yes we did remove it, this is the before/after.
Comment on lines
479
to
480
| // TODO: Switch to `true` (40px size) if possible | ||
| __next40pxDefaultSize={ false } |
There was a problem hiding this comment.
@WordPress/gutenberg-design Do you have a preference on how to upsize the "Content" and "Wide" controls? We can for example break them up into two rows, or remove the icon. There is hint text below it that applies to both controls, which may be harder to visually grasp when broken up into two rows.
There was a problem hiding this comment.
I recently tinkered with this mockup:
What do you think?
Edit: this is in global styles, not per container block, but the content/wide aspect would be the same.
There was a problem hiding this comment.
One drawback with this approach is that we now have decorative icons situated in a location where you often find interactive icon buttons. In other words those icons look clickable.
Could they appear inside the field instead, as is standard practise with similar fields?
There was a problem hiding this comment.
Maybe, though to me they also look clickable there :). They also wouldn't line up with other icons in the panel, though that's secondary since it's alread an issue.
There was a problem hiding this comment.
I find the InputControl prefix slot is usually used for decorative items, whereas the label suffix is usually for interactive items, yes. Ideally I'd like to put it in the prefix slot, but not sure if it'll fit in a two-column layout. I can try.
There was a problem hiding this comment.
I'm not sure about the two rows, being on one row implies these are related, which they are. I think this is worth solving in design before we move that much farther.
Did you consider the 12px padding left/right with a smaller unit selector? The gaps in #64520 (comment) should be possible to reduce quite a bit.
There was a problem hiding this comment.
Ok, if two rows are also not great, I'll leave these controls as is for this PR. The required changes are out of scope.
I'm on board with reducing the input paddings to 12px though, as long as we do it separately and the Figma files are updated 👍 As for the unit selector, I'm not sure we have much to reduce unless we do something a bit more drastic, since this is what we're dealing with:
There was a problem hiding this comment.
Ooh, the new Figma file already does have tighter margins, perfect! Will reflect in code then 👍
There was a problem hiding this comment.
Input padding decrease proposed in #64708.
There was a problem hiding this comment.
Unit select dropdown changes proposed in #64712.
28 tasks
mirka
commented
Aug 15, 2024
| @@ -73,13 +73,13 @@ function CoverHeightInput( { | |||
|
|
|||
| return ( | |||
| <UnitControl | |||
| label={ __( 'Minimum height of cover' ) } | |||
| __next40pxDefaultSize | |||
There was a problem hiding this comment.
| Before | After |
|---|---|
|
 |
Tweaked as discussed in #64520 (comment)
mirka
force-pushed
the
unitcontrol-40px
branch
from
f069805
to
71fbd47
Compare
mirka
commented
Aug 21, 2024
| @@ -628,8 +635,8 @@ export default function DimensionsPanel( { | |||
| /> | |||
| ) : ( | |||
| <UnitControl | |||
| __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. |
|
This is ready for final review @WordPress/gutenberg-components 🙏 The Content/Wide controls will be addressed separately. |
ciampo
approved these changes
Aug 22, 2024
This was referenced Aug 27, 2024
2 tasks
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
* Fix in Featured Image block * Fix in Cover block * Make explicit in LetterSpacingControl * Add TODO comments * Add lint rule * Tweak in Cover block * Move to prefix slot in Dimensions Panel * Tweak padding? * Fix "Block spacing" in Dimensions Panel * Add LetterSpacingControl to linter * Fix tests * Revert "Tweak padding?" This reverts commit 7aabc76. * Revert "Move to prefix slot in Dimensions Panel" This reverts commit 2b3e3a1. * Move to less strict lint * Add link to relevant discussion Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>
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 UnitControl that do not adhere to the new default size.
How?
I migrated all the usages, except for the Content/Wide controls that will require component-level design changes.
Testing Instructions
The lint error should trigger for violations as expected.