Fix UI order for theme.json spacing sizes #62199
Merged
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
ajlende
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-theme-json-gutenberg.php |
|
@ajlende Confirmed that this is now working, per video: Screen.Recording.2024-05-31.at.4.43.03.PM.mov |
|
Size Change: 0 B Total Size: 1.74 MB ℹ️ View Unchanged
|
|
Would love to get this approved and into Beta sooner vs. later, so those testing the Beta won't have broken spacing on both front/back end. |
|
I tested the PR, and it works. But as you mentioned, the labels do not display correctly in the Editor UI, which I would consider a regression from 6.5. Named sizes (small, medium, large, etc.) is a pretty common approach. 
|
talldan
approved these changes
Jun 4, 2024
|
@ajlende Checks out and looks good to me as well. Works as expected. LGTM and get this into Beta 1! |
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jun 4, 2024
See #6616. See also the original Gutenberg PRs: * WordPress/gutenberg#58409 * WordPress/gutenberg#61328 * WordPress/gutenberg#61842 * WordPress/gutenberg#62199 * WordPress/gutenberg#62252 Fixes #61282. Props ajlende, talldanwp, ramonopoly, ellatrix. git-svn-id: https://develop.svn.wordpress.org/trunk@58328 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jun 4, 2024
See WordPress/wordpress-develop#6616. See also the original Gutenberg PRs: * WordPress/gutenberg#58409 * WordPress/gutenberg#61328 * WordPress/gutenberg#61842 * WordPress/gutenberg#62199 * WordPress/gutenberg#62252 Fixes #61282. Props ajlende, talldanwp, ramonopoly, ellatrix. Built from https://develop.svn.wordpress.org/trunk@58328 git-svn-id: http://core.svn.wordpress.org/trunk@57785 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ellatrix
added
the
Backport to WP 6.7 Beta/RC
github-actions bot
pushed a commit
to platformsh/wordpress-performance
that referenced
this pull request
Jun 4, 2024
See WordPress/wordpress-develop#6616. See also the original Gutenberg PRs: * WordPress/gutenberg#58409 * WordPress/gutenberg#61328 * WordPress/gutenberg#61842 * WordPress/gutenberg#62199 * WordPress/gutenberg#62252 Fixes #61282. Props ajlende, talldanwp, ramonopoly, ellatrix. Built from https://develop.svn.wordpress.org/trunk@58328 git-svn-id: https://core.svn.wordpress.org/trunk@57785 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
Works as expected 👌 
|
|
̶L̶G̶T̶M̶ ̶a̶n̶d̶ ̶g̶e̶t̶ ̶t̶h̶i̶s̶ ̶i̶n̶t̶o̶ ̶B̶e̶t̶a̶ ̶1̶!̶ LGTM and get this into Beta 2! |
|
Ugh. These performance tests. They were passing the entire time. Then I changed the code to do less work in 099aaf7. Now they are consistently failing. |
westonruter
added a commit
that referenced
this pull request
Jun 4, 2024
…dd/on-async-directives * 'trunk' of https://github.com/WordPress/gutenberg: (72 commits) Top toolbar: fix half a pixel artifacting of the bottom border. (#62225) Fix UI order for theme.json spacing sizes (#62199) Chore: Simplify a padding style on global styles. (#62291) Editor: Avoid remounts of `DocumentBar` (#62214) Add `default-spacing-sizes` and `default-font-sizes` options for classic themes (#62252) Editor: Cleanup styles and classnames (#62237) Scripts: Pin the @wordpress/scripts version to a version supported by WP 6.5 (#62234) Documentation: Better changelogs for the JSX transform upgrade (#62265) Chore: Simplify a padding style on dataviews. (#62276) MediaUpload: Remove dialog markup on close (#62168) Tabs: Prevent accidental overflow in indicator (#61979) Make edit site pagination buttons accessibly disabled. (#62267) Fix: Remove unused code from dataviews styles. (#62275) Re-enable React StrictMode (#61943) Inserter: Return the same items when the state and parameters don't change (#62263) Update instances of text-wrap: pretty to fall back to balance (#62233) Update: Slotfill documentation samples (links, code, and rephrase). (#62271) Try: Fix mover positioning. (#62226) [Mobile] - Image corrector - Check the path extension is a valid one (#62190) Update: Block styles documentation. ...
|
@ajlende @richtabor @ndiego Any chance this can get added to 18.5, given it's a fix for #61842 which was merged in the 18.5 RC? Otherwise, this doesn't make its way into WP 6.6, which means we wouldn't see this solved until 6.7? |
|
@bgardner It's labeled as 'Backport to WP Beta/RC', so it will be in 6.6, but might not be in the first beta if it's not in 18.5. |
|
Can someone else test this setup with custom slugs? "spacing": {
"defaultSpacingSizes": false,
"spacingSizes": [
{
"name": "Fluid Scale -3",
"size": "clamp(0.31rem, 0.11cqi + 0.28rem, 0.35rem)",
"slug": "minus-3"
},
{
"name": "Fluid Scale -2",
"size": "clamp(0.47rem, 0.16cqi + 0.42rem, 0.53rem)",
"slug": "minus-2"
},
{
"name": "Fluid Scale -1",
"size": "clamp(0.71rem, 0.25cqi + 0.63rem, 0.79rem)",
"slug": "minus-1"
},
{
"name": "Fluid Scale +/- 0 (Base)",
"size": "clamp(1.06rem, 0.37cqi + 0.95rem, 1.19rem)",
"slug": "base"
},
{
"name": "Fluid Scale +1",
"size": "clamp(1.20rem, 0.85cqi + 0.94rem, 1.48rem)",
"slug": "plus-1"
},
{
"name": "Fluid Scale +2",
"size": "clamp(1.34rem, 1.5cqi + 0.89rem, 1.86rem)",
"slug": "plus-2"
},
{
"name": "Fluid Scale +3 (Global)",
"size": "clamp(1.86rem, 3.7cqi + -0.05rem, 2.32rem)",
"slug": "plus-3"
},
{
"name": "Fluid Scale +4",
"size": "clamp(1.86rem, 7.53cqi + -1.2rem, 3.62rem)",
"slug": "plus-4"
},
{
"name": "Fluid Scale +5",
"size": "clamp(1.86rem, 10.2cqi + -2rem, 4.53rem)",
"slug": "plus-5"
}
]
}I'm still seeing both the default sizes and my custom sizes out of order:
Want to make sure I'm not doing something weird before opening another ticket. |
|
I had to bring WordPress trunk build up to date to get the correct order. :) Though, I'm still seeing the default spacing sizes. They're just appended to the bottom of the list. |
@justintadlock I haven't been able to reproduce your screenshot at all. It looks like you're editing the block spacing setting for a group block in the post editor. This is what I see with Gutenberg trunk and WordPress trunk.
I'm not entirely sure what's going on.
That surprises me. WordPress trunk doesn't have the JS changes for the sorting changes yet, so I would expect them to be in the wrong order until the JS gets backported. Do you have an old version of the Gutenberg plugin active by chance? |
|
Thanks, @ajlende, for testing. I think I just need to reset my dev environment. I think I've broken something somewhere. |
ellatrix
pushed a commit
that referenced
this pull request
Jun 11, 2024
Co-authored-by: ajlende <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: bgardner <[email protected]> Co-authored-by: ndiego <[email protected]>
ellatrix
pushed a commit
that referenced
this pull request
Jun 11, 2024
Co-authored-by: ajlende <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: bgardner <[email protected]> Co-authored-by: ndiego <[email protected]>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
Co-authored-by: ajlende <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: bgardner <[email protected]> Co-authored-by: ndiego <[email protected]>
ellatrix
removed
the
Backport to WP 6.7 Beta/RC
|
This was cherry-picked to the wp/6.6 branch. |
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?
Solves the ordering issue for spacing sizing presets in v2 theme.json files that don't follow the
'10','20', etc. convention for slugs.The labels in the range slider remain numbers as, I believe, that was the intention of the feedback that #44247 fixed on the server instead of in the UI. It was a bug if the slider didn't show numbers.The labels are kept as t-shirt sizes when using the slider now.
Why?
Fixes #62194
How?
It works by only sorting presets when they need to be merged.
See #62194 (comment) for details, but to summarize the constraints of the problem:
spacingSizesslugs that do not conform to the number convention must use the ordering defined in the theme.json file.V2 theme.json files will always only have one
spacingSizesarray, either the one defined in theme.json the one generated (in order) fromspacingSizesvalues.V2 theme.json files will always only have one origin of
spacingSizesdefined.V3 theme.json files allow both
spacingScaleandspacingSizesto be defined at the same time and the results merged into one array. When this is done on the server, we can sort the merged values. These merged values becomespacingSizesfor their origin in the frontend.V3 theme.json files allow
default,theme, andcustomorigins forspacingSizesthat are separated until they reach the frontend. Only sort them in the frontend if more than one origin is defined.Testing Instructions
Add this in a
"version": 2theme.json file.The slider should still use numbers, but the presets should remain in their original order.The slider uses the preset names and remain in their original order.
xSmall = var(--wp--preset--spacing--x-small)
Small = var(--wp--preset--spacing--small)
Medium = var(--wp--preset--spacing--medium)
Large = var(--wp--preset--spacing--large)
xLarge = var(--wp--preset--spacing--x-large)
Additionally, v3 files should behave the same when
defaultSpacingSizesis also set tofalse.Testing Instructions for Keyboard
Screenshots or screencast