-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CheckboxControl: replace margin overrides with new opt-in prop #45434
CheckboxControl: replace margin overrides with new opt-in prop #45434
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the amazing testing instructions, it was a breeze to review!
Everything looks good, we can merge once you update the snapshots and add a changelog.
.components-base-control__field { | ||
align-items: center; | ||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR, but a good follow-up for these would be to move the Icon out of the CheckboxControl label
(it doesn't make sense), and move these flex styles into the parent element so we can remove these hacky overrides on components-base-control__field
.
(Same with block-manager
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mirka when I initially read your comment, I assumed it was related to accessibility. I've just tested with my screen reader and looked a bit into it, but I couldn't find anything. Would you mind sharing why it is best to move the icon out of the label (aside from the opportunity to remove the CSS overrides)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it's not an a11y issue, just that there is no reason for the aria-hidden icon to be in the label
. The main motivation is to remove the CSS override. I'll add some context in case some of it was missing:
Any consumer usage that reaches into and overrides the internal styling of a wp-component is inherently brittle. If BaseControl changes its structure or styling at some point, these BlockLock overrides could break in unexpected ways. We've been discouraging style hacks like this, since it actively harms our ability to make changes to wp-components without things breaking all over the place. It's doubly harmful because third-party devs often turn to the Gutenberg source code to learn how they should be building things.
So when things look like you might want to override internal wp-component styling, the decision flow would roughly be like:
- Can I accomplish the same thing without adding a style hack? If so, that is always better.
- Is this a common design pattern that will appear multiple times in the Gutenberg app? If so, consider proposing a change to the wp-component in question so it can flexibly handle more use cases out of the box.
- If no to either of those questions, consider building your own component from scratch, or modifying your design so it works within the existing component limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed explanation! Very helpful! 🙌
e6470f9
to
99ac2c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah never mind about the changelog, this PR doesn't need one 😄
Good to go then! 🚀
What?
Replacing margin overrides with new opt-in prop
__nextHasNoMarginBottom
for useages ofCheckboxControl
in the Gutenberg codebase.Why?
Part of this project: #38730
The tl;dr is
BaseControl
has amargin-bottom
which makes it difficult to reuse and results in inconsistent use.How?
By removing margin overrides in the CSS and adding the prop
__nextHasNoMarginBottom
.To note,
CheckboxControl
inpackages/edit-navigation/src/components/sidebar/manage-locations.js
wasn't updated. I couldn't find this on the front end so I'm unsure if it's being used. It looks like it might be a paused project.Testing Instructions
To test this, you'll need to look for the following checkboxes and ensure that the margin is the same as before (
margin-bottom
of 0) for thediv
with classcomponents-base-control__field
. Screenshots below are how it looks in core right now.For
BlockManagerCategory
andBlockTypesChecklist
For
PostSticky
andPostPendingStatus
For
HierarchicalTermSelector
For
PostComments
andPostPingbacks
For
EntityRecordItem
For
BlockLockModal
For
PostPublishPanel
Screenshots or screencast