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

Jump to bottom

Layout: Fix issue where saving user global styles included layout definitions in layout settings #50268

Merged
merged 1 commit into from
May 3, 2023
Merged

Layout: Fix issue where saving user global styles included layout definitions in layout settings #50268

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
12 changes: 8 additions & 4 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 @@ -62,10 +62,14 @@ export default function DimensionsPanel( { name, variation = '' } ) {
setStyle( updatedStyle );

if ( newStyle.layout !== settings.layout ) {
setSettings( {
...rawSettings,
layout: newStyle.layout,
} );
const updatedSettings = { ...rawSettings, layout: newStyle.layout };

// Ensure any changes to layout definitions are not persisted.
if ( updatedSettings.layout?.definitions ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these definitions? and why do we need them for blocks but not global styles?

Copy link
Contributor

Choose a reason for hiding this comment

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

The definitions live in settings.layout in the core theme.json, and they provide the base styles for each layout type. They're used everywhere layout exists: blocks, root containers and global styles, but they're not meant to be overridden by themes. I suppose it would theoretically be possible for themes to define their own layout rules, but that use case isn't really supported right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for these leaving in "core theme.json" if these are meant to be "static". Can't we just put them in a file in our code base?

Copy link
Contributor

Choose a reason for hiding this comment

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

Initially we were operating on the assumption that these definitions would work like default values, and might be extendable by themes at some point in the future. But looking at it now, layout support logic is perhaps too complex to easily allow for extendability, so it might be better to move these somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Originally, part of the idea of placing the definitions in theme.json was that we might one day be able to make all the layout options declarative and keep the rules in a centralised place so that we could reduce duplication between the JS and PHP implementations of layout. We did reduce some of the duplication by centralising, but as @tellthemachines mentions, the layout support logic is likely too complex to easily support purely declarative layout types. E.g. originally I was wondering if we could get to a point where you might add additional layout types by simply adding an additional definition, and then the editor would know based on that definition which controls to display, etc.

However, that direction is likely a lot more work than the benefits it might yield, and I agree that it could be good to revisit the approach of storing the definitions in theme.json. So, if we were to move them elsewhere, what might that look like? One possible idea:

  • What if we defined the layout definitions in PHP, say hard-coded in layout.php and returned them via get_layout_definitions.
  • Then, in block editor settings, we could add the definitions, JSON-ified, so that the JS code grabs layout definitions from block editor settings.
  • In all PHP usage, we'd use get_layout_definitions instead of pulling from the global styles data.

Would something like that sound do-able / preferable to what we have now? If we like that kind of idea, I'm happy to hack around a bit and see how feasible it is.

In terms of extensibility, if or when we ever wanted to allow themes to extend the layout definitions, we could either add hooks to make the layout definitions filterable, or re-introduce the definitions to theme.json, but grab them from within get_layout_definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, and since these are more implementation details of the layouts instead, I'd move them to the code directly. There's already duplication between a lot of block supports between frontend and backend so I wouldn't mind if they're duplicated too to avoid extra APIs. I also feel like maybe it's a wrong abstraction as I don't see why a layout can be "declarative" but I'll leave that decision to you, the main thing for me is that we should remove these from theme.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, cool, thanks for the feedback. I'll have a play once I'm back from AFK and will explore our options 👍

delete updatedSettings.layout.definitions;
}

setSettings( updatedSettings );
}
};

Expand Down