Normalize block inspector controls spacing #64526
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,10 +11,8 @@ | |
| } | ||
|
|
||
| .components-base-control { | ||
| &:where(:not(:last-child)) { | ||
| margin-bottom: $grid-unit-20; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -26,7 +26,7 @@ | |||||
| } | ||||||
|
|
||||||
| &.is-opened { | ||||||
| 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.
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 😂 |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
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.
I am assuming this style on
:last-childwas an intentional optical adjustment (if not, it would've beenmargin-bottom: 0).I think we should move this optical adjustment to the padding on the containers, i.e.
PanelBodyandToolsPanel. It would be less hacky, and apply more universally (for example if the last control in the container was not wrapped in aBaseControl).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.
While we work on this, could we also find a way to move away from using
.components-base-controlat 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-editorpackage-specific classname instead (ie.block-editor-inspector-controls__controlor something) and apply it where relevant? Or switch toVStackor similar, where the vertical spacing is defined on the parent and not the children?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.
The pathway I have in mind at the moment is:
__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.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.Another long journey 😇
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.
Extracted to a tracking issue: #65191
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.
Shouldn't this be built into the component? Should these panels consume DataForms?
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.