Layout: Fix issue where saving user global styles included layout definitions in layout settings #50268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…initions in layout settings
andrewserong
added
[Type] Regression
andrewserong
requested review from
youknowriad,
jasmussen,
tellthemachines and
aaronrobertshaw
|
Size Change: +33 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
andrewserong
added
the
[Feature] Layout
andrewserong
added
the
Backport to Gutenberg RC
|
I'll be AFK tomorrow, so if this change looks good to land, please feel free to merge it in if anyone would like to! Separately to this PR, we can also explore follow-ups that strip the layout definitions from being returned by the @bph I'm not sure if this PR will make it in, in time, but I've added the Thanks, all! |
|
I just followed the testing instructions on trunk and don't see a layout definitions object in the downloaded theme.json. Maybe something's wrong with my local dev env? |
|
Thanks for testing @tellthemachines — I might have missed a step in the Testing Instructions. I had to save the global styles change (with the |
ramonjd
approved these changes
May 3, 2023
There was a problem hiding this comment.
This works for me. Thanks for the quick fix @andrewserong
Here's what I'm seeing when I export my theme.json changes:
TRUNK
"layout": {
"contentSize": "777px",
"definitions": {
"constrained": {
"baseStyles": [
{
"rules": {
"float": "left",
"margin-inline-end": "2em",
"margin-inline-start": "0"
},
"selector": " > .alignleft"
},
{
"rules": {
"float": "right",
"margin-inline-end": "0",
"margin-inline-start": "2em"
},
"selector": " > .alignright"
},
...
},
"wideSize": "2222px"THIS PR
"layout": {
"contentSize": "777px",
"wideSize": "2222px"
},
tellthemachines
approved these changes
May 3, 2023
There was a problem hiding this comment.
Thanks for updating the testing steps! I must have forgotten to save changes before exporting. It's working for me now: I can see the definitions on trunk, and can confirm that following the same steps on this branch results in no definitions.
Code looks good!
|
Thanks for the reviews, folks, much appreciated! I'll merge this in now, and happy to look into follow-ups for filtering the theme.json data next week if we feel like we'll need it. |
andrewserong
deleted the
fix/layout-persisting-layout-definitions-to-user-settings
branch
|
I just cherry-picked this PR to the release/15.7 branch to get it included in the next release: e1113e5 |
bph
pushed a commit
that referenced
this pull request
May 3, 2023
…initions in layout settings (#50268)
bph
removed
the
Backport to Gutenberg RC
youknowriad
reviewed
May 3, 2023
| const updatedSettings = { ...rawSettings, layout: newStyle.layout }; | ||
|
|
||
| // Ensure any changes to layout definitions are not persisted. | ||
| if ( updatedSettings.layout?.definitions ) { |
There was a problem hiding this comment.
What are these definitions? and why do we need them for blocks but not global styles?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.phpand returned them viaget_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_definitionsinstead 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, cool, thanks for the feedback. I'll have a play once I'm back from AFK and will explore our options 👍
|
Thanks for this PR, the urgent part was ensuring the definitions were not included in theme.json upon theme export. But it'd be good to follow up on Riad's note. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #50238 (potentially)
Update the global styles logic when settings are updated to exclude layout definitions. This will cause the
definitionsobject to no longer be saved within the user data of global styles. For any sites that already have the definition saved in their user data, they can either clear their customizations, or make a change tocontentSizeorwideSizeand re-save, to flush thedefinitionsobject from their user data.Why?
In #48070 the
DimensionsPanelwas refactored to be used across both the block editor and in the site editor / global styles. In that refactor, we missed that before the refactorcontentSizeandwideSizewere updated individually within thelayoutobject, rather than getting and saving the whole layout object when settings were updated. The switch away from this resulted in thedefinitionsobject accidentally being included in the user data.As a result, if a user goes to export their theme, the
definitionsobject was included in the export, since it was included in the user saved styles.How?
definitionsobject.Testing Instructions
contentSizeorwideSize, then save the global styles.ExportunderToolsto download a.zipfile of your theme..zipfile, within thetheme.jsonfile, there should be nosettings.layout.definitionskey.