Skip to content

Layout: show inherit toggle in the absence of settings.layout object #59580

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

Merged
ramonjd merged 1 commit into from
Mar 8, 2024
Merged

Layout: show inherit toggle in the absence of settings.layout object #59580

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 6 deletions packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,13 @@ export function useLayoutStyles( blockAttributes = {}, blockName, selector ) {
const fullLayoutType = getLayoutType( usedLayout?.type || 'default' );
const [ blockGapSupport ] = useSettings( 'spacing.blockGap' );
const hasBlockGapSupport = blockGapSupport !== null;
const css = fullLayoutType?.getLayoutStyle?.( {
return fullLayoutType?.getLayoutStyle?.( {
Copy link
Member Author

@ramonjd ramonjd Mar 5, 2024

Choose a reason for hiding this comment

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

ES lint was bugging me

blockName,
selector,
layout,
style,
hasBlockGapSupport,
} );
return css;
}

function LayoutPanelPure( {
Expand All @@ -144,8 +143,6 @@ function LayoutPanelPure( {
const settings = useBlockSettings( blockName );
// Block settings come from theme.json under settings.[blockName].
const { layout: layoutSettings } = settings;
// Layout comes from block attributes.
const [ defaultThemeLayout ] = useSettings( 'layout' );
const { themeSupportsLayout } = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return {
Expand Down Expand Up @@ -180,11 +177,9 @@ function LayoutPanelPure( {
}

// Only show the inherit toggle if it's supported,
// a default theme layout is set (e.g. one that provides `contentSize` and/or `wideSize` values),
// and either the default / flow or the constrained layout type is in use, as the toggle switches from one to the other.
const showInheritToggle = !! (
allowInheriting &&
!! defaultThemeLayout &&
Copy link
Member Author

@ramonjd ramonjd Mar 5, 2024

Choose a reason for hiding this comment

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

So I don't really know if this is safe.

Things appear to work as expected.

The scenario is that:

  • allowInheriting is true, but
  • defaultThemeLayout has no value.

A value of {} would pass the test, so it seems safe to just wave it through, unless we specifically want to check for settings.layout.wideSize|contentSize?

Not sure 🤔

Copy link
Contributor

@andrewserong andrewserong Mar 5, 2024

Choose a reason for hiding this comment

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

Good question! I haven't taken this PR for a spin, but does this affect classic themes at all? And is it a problem if when you toggle the inherit toggle, if it doesn't immediately apply any rules? I.e. if folks go to set child blocks to "wide" versus content alignment they mightn't see any difference until a value is entered in for the constrained controls 🤔

These are just drive-by comments, though, thanks for working on the fix! If it's still open in a couple of days, happy to take it for a spin and give it a proper review 🙂

Copy link
Member Author

@ramonjd ramonjd Mar 5, 2024

Choose a reason for hiding this comment

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

I haven't taken this PR for a spin, but does this affect classic themes at all?

Good question. I smoke tested a couple and couldn't see any difference between this PR and trunk, but I didn't really go in depth.

I'm just wondering if the approach is any good right now 🤷🏻

And is it a problem if when you toggle the inherit toggle, if it doesn't immediately apply any rules? I.e. if folks go to set child blocks to "wide" versus content alignment they mightn't see any difference until a value is entered in for the constrained controls

I'm not sure - I can't see any issue, but I'm a bit layout blind.

Also, the fact that, on trunk,settings.layout can be an empty object and pass the test is a bit suspect.

If we tighten up that test, so that we need settings.layout to actually contain something, then we'd have to work out how to deal with the empty layout panel.

Whatever folks think is the best way forward...

Copy link
Contributor

@tellthemachines tellthemachines Mar 5, 2024

Choose a reason for hiding this comment

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

Given that this whole UI is going to change once #53455 (or something similar) is ready, it's not too important to make this a perfect fix.

is it a problem if when you toggle the inherit toggle, if it doesn't immediately apply any rules?

I think as long as the controls to set custom content/wide withs are shown, it's not super awkward 😅 because it exposes those controls, which once set will produce the visible effect.

We could perhaps refine the check to make sure that either the theme sets a contentSize or allowCustomContentAndWideSize is true, because if neither exist we might as well hide the whole panel.

( ! layout?.type ||
layout?.type === 'default' ||
layout?.type === 'constrained' ||
Expand Down