SpacingSizesControl: Fix problem with the slider position being reset when saving global styles #50956
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
… when saving global styles
t-hamano
added
[Type] Bug
|
Size Change: +33 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a476121. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5079704923
|
aaronrobertshaw
approved these changes
May 26, 2023
There was a problem hiding this comment.
Thanks for the fix @t-hamano 👍
This is testing well. I could replicate the issue on trunk and this PR fixes it for me.
I found #48070 where this component was affected, but I expect that perhaps before this PR, the object type was also taken into account when decoding.
A git bisect pointed to a different PR that might be the cause: #50366
That PR changed the format of the preset values returned from global styles, so it makes sense that it caused the issue with the spacing control.
It's possible that any other controls testing for the preset values might have been affected. I didn't spot any odd behaviour though testing things like font sizes, font families and the like.
I'm happy to approve this one but feel free to get a second review.
|
@aaronrobertshaw |
|
thank you! |
|
I think this PR introduced an error with some blocks at the block level in global styles, when the |
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: #50940
What?
This PR fixes a problem with the
SpacingSizesControlslider position being reset when the global style is saved.0cb3481674c1b3685a27dcbcb7b54622.mp4
Why?
I will try to explain the cause of this in turn.
When you move a slider in the
SpacingSizesControlcomponent, it holds an object with a CSS variable containing apipeand acolonas values, like this:If you save the global style after moving the slider, an object with the usual CSS variables will be passed to the
DimensionsPanelcomponent as a value, like this:Upon receiving the value, the
DimensionsPanelcomponent attempts to decode the normal CSS variable into a CSS variable containing thepipeandcolonby means of thedecodeValue()function.For Padding, here it is.
gutenberg/packages/block-editor/src/components/global-styles/dimensions-panel.js
Line 258 in f10f85f
However, the
getValueFromVariable()function that thedecodeValue()function calls internally expects the passed value to be of typestringand returns the received value as is if the condition is not met:gutenberg/packages/block-editor/src/components/global-styles/utils.js
Line 307 in f10f85f
Since this value is not a CSS variable containing
pipeandcolon, theSpacingSizesControlcomponent that receives the value will assume it is not a preset value:gutenberg/packages/block-editor/src/components/spacing-sizes-control/utils.js
Line 17 in f10f85f
As a result, the slider position will be lost.
How?
We need to decode the value of each of the objects with
top/right/bottom/leftproperties in some way.I found #48070 where this component was affected, but I expect that perhaps before this PR, the object type was also taken into account when decoding.
I have added a condition and processing to check if the value is an object in the
decodeValue()function, but there might be a better way.Testing Instructions