Prevent layout changes from saving the whole inherited settings object #54500
Conversation
This file contains hidden or 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
youknowriad
added
[Type] Bug
youknowriad
requested review from
aaronrobertshaw,
andrewserong and
stacimc
|
Size Change: +14 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Sep 18, 2023
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for the quick fix! This is testing nicely for me:
✅ Only the updated layout styles are now included in the REST API request
✅ Setting content and wide size layout values at the root level in global styles works as expected and doesn't interfere with any other values as far as I can tell
✅ Code change reads well
I'm not sure if the code path at the screen-block level is ever reached at the moment with current controls, so I left a non-blocking comment there.
LGTM! ✨
Comment on lines
+36
to
37
| const [ userSettings ] = useGlobalSetting( '', undefined, 'user' ); | ||
| const [ rawSettings, setSettings ] = useGlobalSetting( '' ); |
Contributor
There was a problem hiding this comment.
A non-blocking question: why is this grabbing the root settings via '' instead of getting the layout settings via useGlobalSetting( 'layout' )? Is that so that in follow-ups we can expand the range of settings that folks can manage from the global styles UI, rather than having to have separate useGlobalSetting calls for each part of the settings object?
Contributor
Author
There was a problem hiding this comment.
No, that's because the underlying component have been refactored to have always an API that receives the whole "settings" object and the whole "style" object.
Comment on lines
162
to
166
| if ( newStyle.layout !== settings.layout ) { | ||
| setSettings( { | ||
| ...rawSettings, | ||
| ...userSettings, | ||
| layout: newStyle.layout, | ||
| } ); |
Contributor
There was a problem hiding this comment.
In practice, when is this code ever reached? Do we expose layout controls anywhere at the block level in global styles? I don't think we do, but please let me know if I've missed something! The code change here looks good, but I couldn't think of an area where it's currently exposed, so I haven't tested this code path.
Contributor
There was a problem hiding this comment.
Is there any chance this is in preparation for something like section-specific theme.json?
Contributor
There was a problem hiding this comment.
Ah, that could make sense 👍
Contributor
There was a problem hiding this comment.
This code is being reached, though it shouldn't 😅 because even if the contents of newStyle.layout and settings.layout were the same, they're different objects so that condition is always true.
We could do a deep equality comparison but in that case it would be better to compare newStyle.layout with userSettings.layout, because settings.layout for the blocks is always {contentSize: false, wideSize: false} (I assume because the blocks don't currently support those settings)
We don't currently expose layout controls at block level, though that could conceivably change (content/wide size could perhaps be set at global level for certain blocks?). Even so it might be better to scrap all the layout-specific logic in this screen for now and add it in later if/when we need it.
andrewserong
added
the
[Feature] Layout
aaronrobertshaw
approved these changes
Sep 18, 2023
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
This is testing well for me as well 👍
| Before | After |
|---|---|
 |
 |
Comment on lines
162
to
166
| if ( newStyle.layout !== settings.layout ) { | ||
| setSettings( { | ||
| ...rawSettings, | ||
| ...userSettings, | ||
| layout: newStyle.layout, | ||
| } ); |
Contributor
There was a problem hiding this comment.
Is there any chance this is in preparation for something like section-specific theme.json?
Contributor
tellthemachines
left a comment
tellthemachines
left a comment
There was a problem hiding this comment.
Thanks for the PR! The main thing - making sure that theme settings aren't saved as user edits - is working well, but I added a couple comments about the layout comparisons below.
Comment on lines
162
to
166
| if ( newStyle.layout !== settings.layout ) { | ||
| setSettings( { | ||
| ...rawSettings, | ||
| ...userSettings, | ||
| layout: newStyle.layout, | ||
| } ); |
Contributor
There was a problem hiding this comment.
This code is being reached, though it shouldn't 😅 because even if the contents of newStyle.layout and settings.layout were the same, they're different objects so that condition is always true.
We could do a deep equality comparison but in that case it would be better to compare newStyle.layout with userSettings.layout, because settings.layout for the blocks is always {contentSize: false, wideSize: false} (I assume because the blocks don't currently support those settings)
We don't currently expose layout controls at block level, though that could conceivably change (content/wide size could perhaps be set at global level for certain blocks?). Even so it might be better to scrap all the layout-specific logic in this screen for now and add it in later if/when we need it.
| delete updatedStyle.layout; | ||
| setStyle( updatedStyle ); | ||
|
|
||
| if ( newStyle.layout !== settings.layout ) { |
Contributor
There was a problem hiding this comment.
Can we change settings.layout here to userSettings.layout? The same is happening here as with the check in the blocks screen. If we compare against userSettings then at least this code won't run if both are undefined 😄 but it's probably best to do a deep equality comparison too.
Contributor
Author
There was a problem hiding this comment.
No, it's not necessary to do a deep equality for performance reasons IMO but yeah, we should compare to useSettings indeed.
youknowriad
force-pushed
the
update/fix-global-styles-layout-changes
branch
from
a2b8699 to
48ce9c0
Compare
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.
Fixes #53868
What?
When editing the layout config (contentSize for instance) in global styles, only this change need to be persisted in the backend, if you inspect the REST API request, you'll notice that the full settings object including all the settings that were inherited from the default core settings were being saved as "edits". This PR fixes that.
Testing Instructions
1- Edit the layout settings in global styles, trigger a "save" for global styles and ensure that the settings object sent to the server (REST API request) only contains the modified settings (layout).