BaseControl: change label's display: block #63911
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
|
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
added
[Type] Bug
|
Size Change: -110 B (-0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
2 tasks
|
🤔 I'm not sure I know of a way to trim the whitespace associated to Maybe we could change |
|
Thanks for following up! For what it's worth, this has solved the alignment issue in #63886: 
For anyone else following along, the issue described in #9612 can be tested in this PR by using a narrower viewport width with the Typography controls. In this PR, clicking the whitespace to the right of the Appearance label will redirect focus to the component. The earlier fix in #14478 prevented this. However, I notice in Trunk2024-07-25.11.57.13.mp4This PR2024-07-25.11.47.12.mp4In some ways this PR nearly makes things feel a bit more consistent to me, but I don't feel strongly either way about the clickable area problem.
If it's worth it to fix the width of the labels to limit the clickable area, then this sounds worth a try to me! |
|
As a fix for #63886, this works for me too 👍
Given that it appears #9612 wasn't fully solved and we have some inconsistency there on trunk, I'd lean toward fixing the obvious visual alignment and follow up on the trimming of whitespace separately.
This might be the best bet long term to get the best of both worlds. |
Good point 💯 |
I agree with this 👍 Also, confirming that this tests well across various controls in the post and site editor and resolves #63886. |
Mamaduka
approved these changes
Jul 25, 2024
tyxla
approved these changes
Jul 26, 2024
|
Thank you, folks! If the size of the clickable label ever becomes problematic, we can test the suggested approach above:
|
|
Thanks for landing this fix, @ciampo! 🙇 |
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.
What?
Change the label style of
BaseControlfromdisplay: inline-blocktodisplay: blockWhy?
Using
display: inline-blockwouldn't trim the white space around the label, which would cause inconsistent gaps across the editor based on the layout inherited from the parent element rendering the label.Switching to
display: blockensures that the white space is always trimmed consistently.How?
display: inline-blocktodisplay: blockTesting Instructions
The label is publicly exposed via the
BaseControl.VisualLabelcomponent, and internally via theStyledLabelstyled component.Affected components:
BorderControl,BorderBoxControl,FormTokenField,CustomSelectControl,TimeInput,TimePicker,DateTimePicker,FontSizePicker,ToggleGroupControl.I honestly think that there shouldn't be regressions, but I recommend folks to do some smoke testing across Storybook and the editor. I don't think we need extensive testing either — if any layout breaks we will likely find out as we go in the upcoming weeks; it shouldn't compromise the usability of the product and it will be an easy fix.
Also, make sure that #63886 is fixed.