Skip to content
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.

Sign up for GitHub

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

Normalize block inspector controls spacing #64526

Merged
merged 9 commits into from
Sep 11, 2024
Merged

Normalize block inspector controls spacing #64526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
58014a8
Try block inspector controls spacing normalization
mirka Aug 14, 2024
4132ac4
Merge branch 'trunk' into controls-grid-spacing
mirka Sep 6, 2024
0e27c79
Decrease specificity
mirka Sep 6, 2024
9c0a29f
Merge branch 'trunk' into controls-grid-spacing
mirka Sep 10, 2024
7939750
Add changelog
mirka Sep 10, 2024
5dcdcbd
Grid block: Fix when experimental grid interactivity
mirka Sep 11, 2024
a758f2d
Remove adjustment for bottom padding
mirka Sep 11, 2024
67b9a1a
Revert "Add changelog"
mirka Sep 11, 2024
865c053
Remove unnecessary margin on Layout hook toggle (#65133)
mirka Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/block-editor/src/components/block-inspector/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@
}

.components-base-control {
margin-bottom: #{ $grid-unit-30 };

&:last-child {
margin-bottom: $grid-unit-10;
Comment on lines -16 to -17
Copy link
Member Author

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-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).

Copy link
Contributor

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-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?

Copy link
Member Author

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:

  1. Add a __next prop 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 on InspectorControls and not on any @wordpress/components components.
  2. 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 spacing is 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.
  3. Migrate everything in the codebase.
  4. Deprecate the automatic margin-bottom on BaseControl.

Another long journey 😇

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

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?

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&:where(:not(:last-child)) {
margin-bottom: $grid-unit-20;
}
}

Expand Down
1 change: 0 additions & 1 deletion packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,6 @@ function LayoutPanelPure( {
<>
<ToggleControl
__nextHasNoMarginBottom
className="block-editor-hooks__toggle-control"
label={ __( 'Inner blocks use content width' ) }
checked={
layoutType?.name === 'constrained' ||
Expand Down
4 changes: 0 additions & 4 deletions packages/block-editor/src/hooks/layout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,3 @@
margin-bottom: $grid-unit-10;
}
}

.block-editor-hooks__toggle-control.block-editor-hooks__toggle-control {
margin-bottom: $grid-unit-20;
}
29 changes: 16 additions & 13 deletions packages/block-editor/src/layouts/grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
__experimentalUnitControl as UnitControl,
__experimentalParseQuantityAndUnitFromRawValue as parseQuantityAndUnitFromRawValue,
__experimentalVStack as VStack,
} from '@wordpress/components';
import { useState } from '@wordpress/element';

Expand Down Expand Up @@ -84,19 +85,21 @@ export default {
layout={ layout }
onChange={ onChange }
/>
{ showColumnsControl && (
<GridLayoutColumnsAndRowsControl
layout={ layout }
onChange={ onChange }
allowSizingOnChildren={ allowSizingOnChildren }
/>
) }
{ showMinWidthControl && (
<GridLayoutMinimumWidthControl
layout={ layout }
onChange={ onChange }
/>
) }
<VStack spacing={ 4 }>
{ showColumnsControl && (
<GridLayoutColumnsAndRowsControl
layout={ layout }
onChange={ onChange }
allowSizingOnChildren={ allowSizingOnChildren }
/>
) }
{ showMinWidthControl && (
<GridLayoutMinimumWidthControl
layout={ layout }
onChange={ onChange }
/>
) }
</VStack>
</>
);
},
Expand Down
Loading