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

Show child layout controls by default #49389

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Show child layout controls by default #49389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9772f53
Show child layout controls by default
tellthemachines Mar 28, 2023
11df198
Add consistency to control display defaults
tellthemachines Mar 31, 2023
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
43 changes: 30 additions & 13 deletions packages/block-editor/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,13 @@ function DimensionsToolsPanel( {
}

const DEFAULT_CONTROLS = {
contentSize: true,
wideSize: true,
padding: true,
margin: true,
blockGap: true,
minHeight: true,
contentSize: false,
wideSize: false,
padding: false,
margin: false,
blockGap: false,
minHeight: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry I don't understand why we had to change the default values here? Yeah, there's a small potential of changing the UI for existing blocks, but for me "true" makes more sense as default values for all of these.

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, a user of DimensionsControl shouldn't have to define the "defaultControls" prop in order to have a working component.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're worried about the changes for blocks, we can just update the block.json of the affected blocks?

The other piece of feedback I have is that we should be consistent in behavior between all the *Panel component (we have 4 right now I believe)

Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on this? I'm actually not really satisfied that this PR changed the behavior of default controls in one Styles UI component but not the others, I'm also not convinced that this is the right behavior. Global styles are not customizable by the user, so they shouldn't need to pass any "defaultControls" prop, while the "blocks" (block inspector) are the ones that can customize the default controls, so they should be the ones providing a custom value for the defaultControls prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good question @youknowriad. When reviewing this one, I quite liked the idea of the panel itself defining the defaults, and then the site editor/global styles as an exception specifying that it wants to display all the controls. In that way, the defaults at the component level follow the expected behaviour in the block editor. But I can see the goal in preferencing things one way or the other.

Either way, I agree that it'd be good to make sure it's consistent across the different panels. If we're to do that, do you have a preference for how to proceed while maintaining displaying the child layout controls by default?

I've also left a similar comment on the styles UI documentation PR (#49720 (comment)) so that these discussions are connected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason I went with this option was to not change things for third party blocks. Previously, the default was none of the controls would display except for global styles, but the default setting for the controls was to display all. Blocks outside of global styles weren't displaying controls because the defaults were being silently overridden by an empty object, which is non-ideal. But that's how things have been for a while, and if we change the default behaviour now, any custom blocks using these block supports will suddenly have a bunch of controls displaying by default that they didn't have before.

Personally, I'd love to have everything display by default, because having to look for controls in dropdowns is excruciatingly painful UX, but I'm concerned about changing things around for extenders with no warning.

I'm happy to make the same change for the other block supports as long as we're clear this is the way we want to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's not idea to change behavior on block authors but maybe we can make some small adjustments as it's not entirely a breaking change, things still work, they may just be hidden or visible. I think a dev note in this case would be cool.

Maybe we should try to make the change with the least disruption to block authors while making sure the default values are decent and consistent across the components. what would that be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

making sure the default values are decent and consistent across the components. what would that be?

I'd be happy to make them all visible by default all the time, but perhaps best defer to @WordPress/gutenberg-design 😄

childLayout: true,
};

export default function DimensionsPanel( {
Expand Down Expand Up @@ -420,7 +421,10 @@ export default function DimensionsPanel( {
label={ __( 'Content size' ) }
hasValue={ hasUserSetContentSizeValue }
onDeselect={ resetContentSizeValue }
isShownByDefault={ defaultControls.contentSize }
isShownByDefault={
defaultControls.contentSize ??
DEFAULT_CONTROLS.contentSize
}
panelId={ panelId }
>
<HStack alignment="flex-end" justify="flex-start">
Expand All @@ -446,7 +450,9 @@ export default function DimensionsPanel( {
label={ __( 'Wide size' ) }
hasValue={ hasUserSetWideSizeValue }
onDeselect={ resetWideSizeValue }
isShownByDefault={ defaultControls.wideSize }
isShownByDefault={
defaultControls.wideSize ?? DEFAULT_CONTROLS.wideSize
}
panelId={ panelId }
>
<HStack alignment="flex-end" justify="flex-start">
Expand All @@ -471,7 +477,9 @@ export default function DimensionsPanel( {
hasValue={ hasPaddingValue }
label={ __( 'Padding' ) }
onDeselect={ resetPaddingValue }
isShownByDefault={ defaultControls.padding }
isShownByDefault={
defaultControls.padding ?? DEFAULT_CONTROLS.padding
}
className={ classnames( {
'tools-panel-item-spacing': showSpacingPresetsControl,
} ) }
Expand Down Expand Up @@ -510,7 +518,9 @@ export default function DimensionsPanel( {
hasValue={ hasMarginValue }
label={ __( 'Margin' ) }
onDeselect={ resetMarginValue }
isShownByDefault={ defaultControls.margin }
isShownByDefault={
defaultControls.margin ?? DEFAULT_CONTROLS.margin
}
className={ classnames( {
'tools-panel-item-spacing': showSpacingPresetsControl,
} ) }
Expand Down Expand Up @@ -549,7 +559,9 @@ export default function DimensionsPanel( {
hasValue={ hasGapValue }
label={ __( 'Block spacing' ) }
onDeselect={ resetGapValue }
isShownByDefault={ defaultControls.blockGap }
isShownByDefault={
defaultControls.blockGap ?? DEFAULT_CONTROLS.blockGap
}
className={ classnames( {
'tools-panel-item-spacing': showSpacingPresetsControl,
} ) }
Expand Down Expand Up @@ -595,7 +607,9 @@ export default function DimensionsPanel( {
hasValue={ hasMinHeightValue }
label={ __( 'Min. height' ) }
onDeselect={ resetMinHeightValue }
isShownByDefault={ defaultControls.minHeight }
isShownByDefault={
defaultControls.minHeight ?? DEFAULT_CONTROLS.minHeight
}
panelId={ panelId }
>
<HeightControl
Expand All @@ -612,7 +626,10 @@ export default function DimensionsPanel( {
hasValue={ hasChildLayoutValue }
label={ childLayoutOrientationLabel }
onDeselect={ resetChildLayoutValue }
isShownByDefault={ defaultControls.childLayout }
isShownByDefault={
defaultControls.childLayout ??
DEFAULT_CONTROLS.childLayout
}
panelId={ panelId }
>
<ChildLayoutControl
Expand Down
11 changes: 11 additions & 0 deletions packages/edit-site/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ const {
DimensionsPanel: StylesDimensionsPanel,
} = unlock( blockEditorPrivateApis );

const DEFAULT_CONTROLS = {
contentSize: true,
wideSize: true,
padding: true,
margin: true,
blockGap: true,
minHeight: true,
childLayout: false,
};

export default function DimensionsPanel( { name, variation = '' } ) {
let prefixParts = [];
if ( variation ) {
Expand Down Expand Up @@ -66,6 +76,7 @@ export default function DimensionsPanel( { name, variation = '' } ) {
onChange={ onChange }
settings={ settings }
includeLayoutControls
defaultControls={ DEFAULT_CONTROLS }
/>
);
}