Update help text alignment in CheckboxControl #60787
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
jameskoster
added
[Type] Enhancement
|
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: +91 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
tyxla
reviewed
Apr 16, 2024
t-hamano
reviewed
Apr 17, 2024
|
I very much share the instinct here to make consisten across toggle and checkboxes. Either make consistent, or establish a system for when the rules are broken. As Marin mentions, right now we do both for the switch:
The toggle is meant to be a bit more substantial than the checkbox, se the heuristic could be this:
In all cases it would be good to stress a bit more: what happens when the label itself spans 2 lines of text? What about the help text spanning 2+ lines of text? What if both happen? We can establish best practices for these, but as primitives those rules will occasionally be broken. |
|
It seems reasonable for radio + checkbox to be consistent, especially as the inputs share dimensions. Toggles are potentially one to handle separately as there are so many different applications currently (including some where the toggle is right aligned). |
Do we actually have anything outside of mockups that do this? I forgot! |
|
Yup, plugin-inserted blocks have a right-aligned-toggle UI. #54029 |
|
A potential draw-back to applying this to 
Any thoughts on that? |
I misread a bit here, because I see a few things going on.
That heuristic would solve it, no? In your example above, the help text should be for the group of radios, and not be indented or attached to the last item. |
|
Yup, I agree. In the future we can aim for something compositionally like this: 
But that's a separate endeavour as RadioControl doesn't allow help text for individual options. |
|
Pushed one more tweak to:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Yes it would be good to bake that into the toggle control, if that's the direction in which we want to head. I'd prefer to explore that one separately though as it's a more widely used component. |
mirka
reviewed
Apr 17, 2024
There was a problem hiding this comment.
Thanks for working on this!
Backlinking as a fix to #58564 (comment)
Comment on lines
9
to
11
| .components-base-control__help { | ||
| margin-left: calc(var(--checkbox-input-size) + #{$grid-unit-15}); | ||
| } |
There was a problem hiding this comment.
This is inevitable at the moment, but raised as an issue at #60836.
There was a problem hiding this comment.
Saw the linked issue. It probably wouldn't be as flexible, but could checkbox control do something like this to indent the text?
help={ <span="components-checkbox-control__help-text">{ helpText }</span> }.components-checkbox-control__help {
margin-left: calc(var(--checkbox-input-size) + #{$grid-unit-15});
}There was a problem hiding this comment.
Oh, good one! Yes that could work in this instance, with a display: inline-block to account for when the help text is long enough to wrap.
There was a problem hiding this comment.
I'm not sure I understand the benefit to doing this? Feel free to push any changes :)
There was a problem hiding this comment.
Good question. The point is that we shouldn't be using selectors from other components, because they aren't guaranteed to be stable. Ideally they would all be obfuscated/randomized so they aren't even available outside the original component.
I'll try it out and push the changes 👍
Comment on lines
13
to
16
| .components-base-control__field { | ||
| // Emsure label doesn't wrap beneath checkbox | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
We can avoid reaching into BaseControl internals here by wrapping our elements in an <HStack spacing={ 0 } justify="start" alignment="top"> instead.
Comment on lines
18
to
21
| .components-checkbox-control__label { | ||
| // Ensure label is aligned with checkbox along X axis | ||
| line-height: var(--checkbox-input-size); | ||
| } |
There was a problem hiding this comment.
Let's move this part out of the .components-checkbox-control block to avoid unnecessary specificity.
Comment on lines
70
to
71
| height: var(--checkbox-input-size); | ||
| aspect-ratio: 1; |
There was a problem hiding this comment.
Was this height necessary? It makes the aspect-ratio redundant.
I think we also need a flex-shrink: 0 here to prevent squishing:
There was a problem hiding this comment.
It was, but moving to a HStack seems to have made it redundant :)
Ah, I was confusing |
|
Not a blocker for the scope of this PR, but the line heights are a bit incongruous at the moment. 
|
|
I think that's because help text has no line-height set at all. It's not so bad in the product because it inherits from the main paragraph style, but it would be good to follow up with a specific declaration. |
mirka
approved these changes
Apr 19, 2024
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?
Update the margin applied to help text in checkbox control so that it aligns with the label.
Why?
improves readability, and aligns with the treatment of status radios in the following UI:
Testing Instructions
Run
npm run storybook:devNavigate to CheckboxControl and check the text alignment
Open the site editor
Navigate to the details page for the "Blog Home" template (you may need to create this template)
Observe the updated text alignment in the "Discussion" settings.
Before
After