Normalize block inspector controls spacing #64526
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
mirka
added
[Type] Enhancement
mirka
commented
Aug 15, 2024
Comment on lines
-16
to
-17
| &:last-child { | ||
| margin-bottom: $grid-unit-10; |
There was a problem hiding this comment.
I am assuming this style on :last-child was an intentional optical adjustment (if not, it would've been margin-bottom: 0).
I think we should move this optical adjustment to the padding on the containers, i.e. PanelBody and ToolsPanel. It would be less hacky, and apply more universally (for example if the last control in the container was not wrapped in a BaseControl).
There was a problem hiding this comment.
While we work on this, could we also find a way to move away from using .components-base-control at all?
Apart from the fact that is a private API of the components package and that it makes the UI fragile to future changes, it also doesn't work for controls that are not wrapped in base control.
Maybe we can introduce a block-editor package-specific classname instead (ie. block-editor-inspector-controls__control or something) and apply it where relevant? Or switch to VStack or similar, where the vertical spacing is defined on the parent and not the children?
There was a problem hiding this comment.
The pathway I have in mind at the moment is:
- Add a
__nextprop and/or data attribute so certain container components can opt out of the automatic margin-bottom on BaseControl. Ideally we'd expose the prop only onInspectorControlsand not on any@wordpress/componentscomponents. - Figure out a standard way we can recommend to WP devs for spacing out their inspector controls. Is it VStack (not ideal because the current default
spacingis 8px instead of the 16px we want in the inspector)? Is it a new, dedicated layout component (Implement a standard "controls grid" component for sidebar #43423)? To be decided. - Migrate everything in the codebase.
- Deprecate the automatic margin-bottom on BaseControl.
Another long journey 😇
There was a problem hiding this comment.
Extracted to a tracking issue: #65191
There was a problem hiding this comment.
Figure out a standard way we can recommend to WP devs for spacing out their inspector controls.
Shouldn't this be built into the component? Should these panels consume DataForms?
There was a problem hiding this comment.
Shouldn't this be built into the component?
We can consider it, for example making the container component a flexbox with a 16px gap. Not immediately sure if that would be too restrictive though.
Should these panels consume DataForms?
Hmm, interesting. @oandregal Are they meant for the block inspector at all? My first impression is that it could be used for some cases, but it wouldn't cover everything.
There was a problem hiding this comment.
Hmm, interesting. @oandregal Are they meant for the block inspector at all? My first impression is that it could be used for some cases, but it wouldn't cover everything.
I presume any place that uses a complex form is a potential consumer of DataForm. I don't think it is ready for replacing inspector controls yet.
|
Size Change: -11.9 kB (-0.67%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
This was referenced Sep 6, 2024
|
Flaky tests detected in 0e27c79. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10746721743
|
5 tasks
|
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. |
mirka
changed the title
jameskoster
reviewed
Sep 10, 2024
| @@ -26,7 +26,7 @@ | |||
| } | |||
|
|
|||
| &.is-opened { | |||
| padding: $grid-unit-20; | |||
| padding: $grid-unit-20 $grid-unit-20 #{$grid-unit-20 + $grid-unit-05} $grid-unit-20; | |||
There was a problem hiding this comment.
Appreciate it's out of scope but do you know why is the bottom padding larger than the rest?
There was a problem hiding this comment.
I'm assuming it was meant as an optical adjustment (see also #64526 (comment)). We can easily remove the larger bottom padding as part of this PR if the design team prefers that.
There was a problem hiding this comment.
It seems unnecessary to me, and doesn't feature in the specs. I don't have the full history but would lean towards removing it.
There was a problem hiding this comment.
FWIW this felt weird when reviewing it. If there's no good reason to have it, let's use the opportunity to remove it.
There was a problem hiding this comment.
Sure, we can remove it then.
| Before | After |
|---|---|
 |
 |
For the record, I do understand why someone would want to add an optical adjustment here. The bottom padding looks more equal to the side padding for some reason 😂
|
I tested lots of blocks and only spotted a small issue in Grid: 
|
tyxla
approved these changes
Sep 10, 2024
I did some code sleuthing and deduced that @jameskoster has the Grid Interactivity experiment enabled 😆 Fixed in 5dcdcbd. After
|
This reverts commit 7939750.
97 tasks
mirka
added
has dev note
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.
Part of #65191
What?
This is an attempt to normalize the controls grid spacing in the Block Inspector, according to the current specs (16px gap).
TODO: Known issues
Why?
The grid spacing of control elements in the Block Inspector is a mess right now.
Most normal controls that are built with
BaseControlrely on a margin-bottom that is set within the.block-editor-block-inspectorscope. It uses an old row gap value of 24px, and attemps to apply an optical adjustment on the final BaseControl. (Which doesn't always work as intended if you're nesting divs 😅)gutenberg/packages/block-editor/src/components/block-inspector/style.scss
Lines 13 to 19 in cf8aeba
This was probably convenient at the time, when flexbox was yet to be widely available. Now, it is hindering our ability to use modern layout methods like
flexandgrid.ToolsPanelhas its own spacing adhering to the new specs (16px row gap), while overriding the aforementioned margin-bottom styles so it can handle the layout withgrid.We need to get this under control, and the first step is to make the spacing values consistent. (Aside: The next step I think is to add a prop and/or data attribute so certain container components can opt out of the automatic margin-bottom on BaseControl.)
How?
See code comments.
Testing Instructions
This should make most block inspector controls look more harmonious, but there will inevitably be a few edge cases where it breaks some existing style override. I'll do my best to catch them, but it would help if testers could smoke test as many blocks as possible.
Note
These changes will theoretically only affect controls that are not part of a
ToolsPanel. So please focus the testing on non-ToolsPanel controls.ToolsPanels have these dropdown menus:
Note
The Layout toggle in blocks like Columns, Post Content, Post Template, and Query currently have an extra margin that will be fixed by #65133.
Screenshots or screencast
Non-exhaustive list of examples:
Audio block
Grid block
Categories List block
✍️ Dev Note
Decreased spacing between Block Inspector controls
The standard spacing between controls in the Block Inspector has been reduced to 16px (was previously 24px).
Extenders will likely see the majority of their controls adhereing automatically to the new spacing, due to a Block Inspector style setting bottom margins on all components built with
BaseControl. However, if you have a control that is not built withBaseControl, or have overridden the bottom margin styles for whatever reason, you may want to adjust your custom spacing so it is consistent with the new 16px spacing.