Fix unlabeled Spacer block controls #63806
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
afercia
added
[Type] Bug
|
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. |
|
Size Change: +222 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Reading the documentation on It looks like BaseControl expects a child element that's an input; but this is a child that's a component, which is a bit different. |
|
Yes, to my understanding the BaseControl is a sort of wrapper used to generate labels and descriptions. That is already used for most (all?) of the input controls internally. In this case, the UnitControl renders a @WordPress/gutenberg-components when you have a chance, could you please clarify how the BaseControl is supposed to be used? In this case, seems to me it was used incorrectly and I think there are more cases where its usage is unclear or incorrect. Which makes me think the BaseControl usage should be audited and the documentation improved. |
In this particular case, it's completely unnecessary in the sense that it can be accomplished with just a UnitControl, but not strictly "incorrect". BaseControl can be nested safely if you know what you're doing. This is the rendered output of this code path, and it's labeled visually and accessibly as expected: 
(Should we add a I do agree that the BaseControl docs are a bit sparse and could use some improvement! |
Sure. There's a problem though as the
|
|
Last commit adds the 40px size to the conditionally-shown UnitControl. Max-width increased to 50%. Screenshot: Reminder: these two controls are rendered conditionally, the UI doesn't show them at the same time.
|
tyxla
reviewed
Jul 24, 2024
| isResetValueOnUnitChange | ||
| min={ MIN_SPACER_SIZE } | ||
| onChange={ handleOnChange } | ||
| style={ { maxWidth: '50%' } } |
There was a problem hiding this comment.
The 50% width still feels arbitrary. I wonder if it should have the same width as it has when it has the slider control.
There was a problem hiding this comment.
The
50%width still feels arbitrary. I wonder if it should have the same width as it has when it has the slider control.
I considered that. Alas, that width is something like 104.some decimals pixels because it's determined by the flax layout. Not a real number we can use. I'd agree 50% is still arbitrary... but it is a little better 😄
There was a problem hiding this comment.
Aside consideration: this is the kind of problems to face when the design asks for something the base components aren't designed for. Local overrides, custom, ad-hoc implementation should generally be avoided.
There was a problem hiding this comment.
If we want to place it on a standard 2-column controls grid, I would recommend something like this:
diff --git a/packages/block-library/src/spacer/controls.js b/packages/block-library/src/spacer/controls.js
index 97e47e4d15..7d09af9969 100644
--- a/packages/block-library/src/spacer/controls.js
+++ b/packages/block-library/src/spacer/controls.js
@@ -56,17 +56,18 @@ function DimensionInput( { label, onChange, isResizing, value = '' } ) {
return (
<>
{ ( ! spacingSizes || spacingSizes?.length === 0 ) && (
- <UnitControl
- id={ inputId }
- isResetValueOnUnitChange
- min={ MIN_SPACER_SIZE }
- onChange={ handleOnChange }
- style={ { maxWidth: '50%' } }
- value={ computedValue }
- units={ units }
- label={ label }
- __next40pxDefaultSize
- />
+ <div className="block-library-spacer__controls-grid">
+ <UnitControl
+ id={ inputId }
+ isResetValueOnUnitChange
+ min={ MIN_SPACER_SIZE }
+ onChange={ handleOnChange }
+ value={ computedValue }
+ units={ units }
+ label={ label }
+ __next40pxDefaultSize
+ />
+ </div>
) }
{ spacingSizes?.length > 0 && (
<View className="tools-panel-item-spacing">
diff --git a/packages/block-library/src/spacer/editor.scss b/packages/block-library/src/spacer/editor.scss
index e0c700795c..f2ca2ef985 100644
--- a/packages/block-library/src/spacer/editor.scss
+++ b/packages/block-library/src/spacer/editor.scss
@@ -46,3 +46,9 @@
height: 100% !important;
}
}
+
+.block-library-spacer__controls-grid {
+ display: grid;
+ grid-template-columns: repeat(2, 1fr);
+ gap: 16px;
+}(Ideally I want some kind of standard component to handle this layout #43423)
However, seeing that the other UnitControl is full-width, I guess this one could just be full-width as well.
There was a problem hiding this comment.
I want some kind of standard component to handle this l
Yes that would be better. I can try your CSS workaround but that's in some way an ad-hoc, custom override that should be avoided.
seeing that the other UnitControl
The linked image gives a 404 error.
There was a problem hiding this comment.
Oops, I was trying to link to this screenshot that was in your original issue:
afercia
force-pushed
the
fix/spacer-block-controls-missing-labeling
branch
from
5a2e15b to
995435b
Compare
This was referenced Jul 24, 2024
|
In the last commit I'm trying the Grid component to make the single UnitControl width be the same of the full one. Screenshot with both controls rendered, for comparison:
However, in my opinion w e shouldn't do this as it would be a unique case implementation, locally overriding the default layout of these controls. |
I tend to agree with you; since we've been trying to reduce special cases and overrides, it might be a good idea to avoid it altogether. Perhaps we can do what @mirka suggested above:
(although I'm not sure which one she was referring to). |
|
Full width sounds good to me, thanks. On it. |
|
The single unit control is now full width. Screenshot:
|
mirka
approved these changes
Jul 25, 2024
|
Thanks all for your reviews and feedback. |
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.
Fixes #63800
What?
Two Spacer block controls are unlabeled.
Why?
All form controls must be labeled.
How?
labelprop to theUnitControlinFlexControls.labelprop toRangeControlinSpacingInputControl.BaseControlwrappingUnitControlin the Spacer block inspector controls.Queation regarding the last point: I noticed other cases where a
BaseControlwraps another control. I'm not sure to understand why. Any clue? Anything I may be missing?Cc @WordPress/gutenberg-components
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast