ToggleControl: remove margin overrides and add opt-in prop #47866
Conversation
|
Size Change: -74 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2cb814c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4298422877
|
|
Thanks for the PR, and thanks for the attention to detail 🙏 Looks much better in the case of the post date format toggle:
I'll refer to you and the components group on the details of the technical implementation, so the following is more of a question to satisfy my curiosity. Looking at the margins of the controls in the inspector, there are a few margins stacking and collapsing, like so:
Would it be possible to use |
Thanks for taking a look @jasmussen ! I would love to use gap here instead. But right now the inconsistent spacing is due to the Inspector's The only way we could change it, in this case, is if we added another CSS override for the Post Date block to remove the
The CSS override is added to make the date picker toggle look consistent with the following two toggles, and so the
|
Right, let's definitely not make overrides just for the post date use of the base control, definitely suggesting a refactor of the control overall. The way I see it is that we are improving the WordPress component set for the long haul. In some cases that means revisiting past decisions, and making breaking changes. In the case of npm consumers, a clear readme before they npm update feels like a potential path forward. Not ideal of course, something to try and avoid when possible, but if we feel with high confidence that the path forward (in this case, gap instead of bottom margins) is the right one, then it is almost certainly going to be worth it from a pure maintenance and code quality perspective. As far as spacing that third party blocks rely on, is there any way we can quantify how big of an issue it is? If we were to make a refactor across all usage of base control, would it be possible to identify problematic issues, and add a temporary hack to address those issues in the component itself? A hack with an end-date, something like that? |
Those are all very good questions! I'm curious to hear what @mirka and @ciampo think about your questions/suggestions. 😄 To me, it sounds like it would be similar to the project this PR is a part of (#38730): adding an opt-in prop, migrating usages in Gutenberg, and then adding the deprecation warning. Or are there limitations on doing this for the inspector's bottom margin (aside from how time-consuming it would be 😅)? |
Honestly I'm thinking even simpler: just making the change. No props, just better code and making the change in one sweeping way. Keep in mind I'm not a developer, and before doing this it would be good to boost the confidence on what effect this might have on 3rd party plugins. As in, try a couple of the most popular block editor plugins that use the base control, see what the patch might do. Mainly I have a feeling that it won't make a big impact, if any. Of course, I might be wrong! But even if there's a teensy bit of visual-only margin breakage in consumers that haven't used the base control correctly, it feels worth it for the long term gain. |
|
Unfortunately we can't just rip off the bandaid because the impact will be substantial. Just looking at usages within core Gutenberg, the majority of inspector controls seem to be relying on the implicit margin-bottom added by the block-inspector stylesheet. What we'll likely do is, once we implement the standard "controls grid" component (#43423), we'll counteract the block-inspector margin overrides inside the CSS scope of that grid. (Similar to how we're starting to do it with higher-level components like FocalPointPicker and QueryControl.) So something like this in the block-inspector stylesheet: .components-controls-grid .components-base-control {
margin-bottom: 0;
}This will help ensure a smooth transition by letting devs opt into the clean and consistent spacing, while also maintaining the legacy usages of those relying on the implicit margin-bottom. |
| label={ | ||
| <> | ||
| { __( 'Default format' ) } | ||
| <span className="block-editor-date-format-picker__default-format-toggle-control__hint"> |
There was a problem hiding this comment.
Can we also remove the associated CSS?
There was a problem hiding this comment.
Yes, good catch! Will update this, rebase, and then merge.
116ddc3 to
2cb814c
Compare
What?
Added new opt-in prop
__nextHasNoMarginBottomfor usages ofToggleControlin the Gutenberg codebase and removed margin overrides.Why?
Part of this project: #38730
The tl;dr is
BaseControlhas amargin-bottomwhich makes it difficult to reuse and results in inconsistent use.How?
By removing margin overrides in the CSS and adding the prop
__nextHasNoMarginBottom.ToggleControlis a bit different compared to the other 'Control' components in the above-mentioned project. There isn't a margin override aside from the one CSS one inBlockLockModal. So there won't be any visible changes to most of these blocks/components. The ones that have visual changes from trunk will be in the testing instructions below.Testing Instructions
In the block editor:
BlockLockModal
DateFormatPicker
In Storybook:
URLPopover
run npm storybook:dev