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

Changes from 1 commit
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
7 changes: 6 additions & 1 deletion 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 @@ -189,6 +189,7 @@ const DEFAULT_CONTROLS = {
margin: true,
blockGap: true,
minHeight: true,
childLayout: true,
};

export default function DimensionsPanel( {
Expand Down Expand Up @@ -612,7 +613,11 @@ export default function DimensionsPanel( {
hasValue={ hasChildLayoutValue }
label={ childLayoutOrientationLabel }
onDeselect={ resetChildLayoutValue }
isShownByDefault={ defaultControls.childLayout }
isShownByDefault={
typeof defaultControls.childLayout === 'boolean'
Copy link
Contributor

@youknowriad youknowriad Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why it's not always a boolean?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding properly, the check is less about type "boolean" and more about checking if the value is actually set to avoid considering "undefined" as "false".

I think I'd prefer if we stay consistent between all the default controls. We're considering undefined as false for all the default controls in all the *Panel components. So my question is: can we just keep it as is here? and update the "block.json" files for the blocks that override the default defaultControls instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding properly, the check is less about type "boolean" and more about checking if the value is actually set to avoid considering "undefined" as "false".

Yes, that's the point of this check.

We're considering undefined as false for all the default controls in all the *Panel components. So my question is: can we just keep it as is here? and update the "block.json" files for the blocks that override the default defaultControls instead?

That's exactly what this change addresses: we need to have a way of defaulting to true for the child layout controls, but still preserving the ability to override that if needed by setting it to false in block.json. Child layout controls can be applied to any block, including third party blocks, so it's not practical to explicitly set the controls to display for every single block type.

Copy link
Contributor

@andrewserong andrewserong Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I like the idea of exposing false as a valid value, and the idea that we can have some controls be opt-out (in terms of the default controls) rather than opt-in.

If consistency for each of the controls is important, what if we used something like isShownByDefault={ defaultControls.childLayout ?? FALLBACK_DEFAULT_CONTROLS.childLayout } for each of the controls. Then, FALLBACK_DEFAULT_CONTROLS could contain the global defaults we'd like to go with. So if all the others are hidden by default, it might be:

const FALLBACK_DEFAULT_CONTROLS = {
	...
	padding: false,
	margin: false,
	blockGap: false,
	minHeight: false,
	childLayout: true,
};

Would something like that work? The idea's basically the same as this PR, but just that we could use the same approach as this PR for each of the other controls, with a const to contain our desired defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

isShownByDefault={ defaultControls.childLayout ?? FALLBACK_DEFAULT_CONTROLS.childLayout }

This works for me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I'll give it a go. Note that doing that will also require us to explicitly set all the controls to true on the site editor side, given that currently the site editor isn't passing any defaultControls into the Dimensions panel, so it depends on the values from DEFAULT_CONTROLS to be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've added the suggested changes!

? defaultControls.childLayout
: DEFAULT_CONTROLS.childLayout
}
panelId={ panelId }
>
<ChildLayoutControl
Expand Down