CheckboxControl: Change alignment #58564
Closed
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
[Package] Block editor
|
Size Change: -3 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
getdave
added
[Feature] Link Editing
draganescu
approved these changes
Feb 1, 2024
getdave
reviewed
Feb 1, 2024
There was a problem hiding this comment.
Thanks for the PR. As of now, by default this is a very minor visual tweak and doesn't cause any problems.
However...if in the future we make these settings extensible, which we may well wish to do, then apply center alignment might be less desirable (depending on how you view such things).
With change (this PR)
Original (trunk)
Therefore, whilst I appreciate the attention to detail, we should consider the potential for knock on effects in the future.
I'll be clear that this is a loosely held opinion so let's wait for other opinions before deciding.
Thanks again for the PR 👍
|
Thanks all for the review. @getdave good point thanks. On one hand, I see the editor uses a vertically centered alignment in many places, see screenshot below where regardless it's one-line or multi-line, elements are vertically centered:
On the other hand, specifically to checkboxes, it appears there is some inconsistency. With the default styling, multi-line labels don't look that great:
The link settings checkboxes case is an exception because the container is set to be a flex container with some ad hoc CSS only for this instance. Basically, some styling from the Looking at the comment in the CSS:
I seem to understand the intent was exactly to avoid the default behavior of the label wrapping under the checkbox. At this point I wonder whether this should be the default behavior and whether flex should be used for the base component instead of local, single use, ad-hoc cases. I'd tend to think 'local' overrides of the base components styling should not be allowed. It's a non-ideal practice that contributors and reviewers should not follow, as it leads to an inconsistent UI over time. If a styling variant is desired, that should be evaluated for general use and if found valid be implemented in the base component. Well, at least this tiny PR surfaced an interesting discussion, I'm curious to hear other thoughts from design and components maintainers Cc @WordPress/gutenberg-design @ciampo
|
|
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. |
I agree with this statement — style overrides are not ideas:
A better approach IMO would be to:
For this specific case, we should discuss these two points:
And understand the next steps. We could either make changes to the component that apply to all existing consumers, or consider introducing props / variants. (also cc'ing @mirka as I'm about to be AFK for a few months) |
I tend to agree. Labels likely shouldn't wrap below the checkbox itself. |
|
Agreed. As an incremental improvement to the component, how about we take this opportunity to align both the label and the help text to the same left edge? 
I think we can consider this a non-breaking change. And since I can't imagine anyone wanting the label to wrap, or the help text to specifically not be aligned with the label, I think this can just be our new default. If @WordPress/gutenberg-design is ok with it, that is 🙂 |
|
Thanks all for the interesting discussion. I'd agree overrides shouldn't be allowed. I'd even tend to think that CSS selectors that belong to base component like Regarding this PR, I'd be in favor of removing the as long as there is a follow-up to implement the new default in the base component. |
mirka
changed the title
|
This is being worked on as a global fix at #60787 |
|
Cloing in favor of #60787 |
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 #58563
What?
Very tiny PR: The vertical alignment of checkboxes and labels in the URL input UI could be slighly improved by using flexbox align-items.
Why?
Flexbox handles vertical alignment nicely and it should be used for that whenever possible.
How?
Set
align-items: centeron the flex container.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before and after: