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.

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

Spacing presets: switch to using numbers instead of t-shirt sizes for labels #44247

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Sep 19, 2022

What?

Replaces X-Small, Small, Medium labels with simple 1,2,3,4 labels for spacing presets

Why?

Requested as a follow-up change here.

How?

Updates the set_spacing_sizes() to assign numbers as the name value of each spacing size

Testing Instructions

  • Add a Group block and set the padding and margin spacing and check that the number labels display as expected
  • Check that other spacing size related functionality still works as expected in editor, frontend and global styles
  • Update settings.spacing.spacingScale.steps to be greater than 7 and check that the select list version of the components still shows t-shirt sizes

Screenshots or screencast

Before:

spacing-sizes-before.mp4

After:

spacing-sizes-after.mp4

@glendaviesnz glendaviesnz added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Sep 19, 2022
@glendaviesnz glendaviesnz self-assigned this Sep 19, 2022
@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Sep 19, 2022

One issue with moving to this format may be that the spacing sizes are generated up and down from a midpoint of 50 in order to allow for better cross-site/theme fallbacks if copied blocks or patterns use spacing preset values not supported by the current theme, ie. if different spacing scales all have a common medium point in the scale we are more likely to get a comparable match when content is moved between sites. So, one drawback of switching from t-shirt sizing to digits is that there now appears to be a vague correlation between the label and the slug, eg. on this PR if you set a value of 1 you will get a CSS var of var(--wp--preset--spacing--20) applied, as the default Core scale only has 3 steps either side of the 50 medium step. This may or may not be a problem, but could potentially be slightly more confusing than a 2x-small label and var(--wp--preset--spacing--20) combination where there is no potential correlation between the label and slug.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing well for me in the post editor, global styles, and on the site front-end, and the code change looks good — also nice that this can be done in a way that preserves the existing slugs, so there aren't any back compat issues with blocks / sites already out in the wild using these presets 👍. So in terms of the code change perspective, and given the time sensitive nature of the next WP release, this LGTM.

However, in terms of personal taste, personally I much preferred the t-shirt size names as they intentionally obscured a numerical representation of the spacing value. One of the challenges of these smaller numbers is that there's no relationship between the number and the real value used, so it can be confusing if you're switching between a custom value and the preset. It isn't immediately clear what the preset value means — as user, I'd think this was a bug in that the value presented does not match the real numerical value in use:

2022-09-19 17 22 26

I believe this is mostly likely a decision for designers, but my personal preference would be to keep the t-shirt sizes instead of using the numerical labels for the presets.

What do you think?

@jasmussen
Copy link
Contributor

This is very likely something wrong with my own environment, so probably safe to dismiss; I can only see multiples of ten, both in trunk and in this branch. I have ghosts in my machine, right?

before

Based on the reviews above, and the videos (thank you), this change looks good, though!

I believe this is mostly likely a decision for designers, but my personal preference would be to keep the t-shirt sizes instead of using the numerical labels for the presets.

The t-shirt sizes made sense to me for typography as we had a rather specific behavior with a segmented control with at most 5 entries lest it become a dropdown. That interface allowed a theme curation aspect, which was well discerned with the t-shirt sizes.

In the case of using the stepped slider, pairing the t-shirt sizes with sequential notches on the slider, feels like a bit of a disconnect. The slider is meant to indicate the range on a specific scale in steps, so for me it feels much more appropriate to show the step on the range, rather than the a specific label.

@jasmussen jasmussen self-requested a review September 19, 2022 09:58
@ramonjd
Copy link
Member

ramonjd commented Sep 20, 2022

Working as advertised for me too. Frontend inline CSS is as expected.

Also checked with some block code saved on trunk and saw no regressions switching to this branch.

Before

2022-09-20 09 51 50

After

2022-09-20 09 34 35

I'll echo sentiments around the number. It might be just my brain, but when I see "5" for example, I ask myself: "5 what?" And relative to what?

It doesn't take long to understand that "5" is larger than "4" and smaller than "6", and that the numbers will mean different things in different themes, e.g., "2" could be huge in one theme and small in another.

I'm just trying to think what I would say to an editor/designer: "Hey, can you set padding to 5?" and how I might need to qualify that request with those unfamiliar with the control.

Anyhoo, it LGTM and we can always iterate, gather feedback etc 🚀

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Sep 20, 2022

I agree with @ramonjd & @andrewserong that there is potential for confusion around the numbers more so than with the t-shirt sizes.

The numbers really don't work when there are more than 8 steps in the scale and it falls back to the select list:

Screen Shot 2022-09-20 at 9 48 51 AM

I have updated the PR so it shows the numbers in the slider and t-shirt sizes for the select list which I think works better than forcing the range style labelling onto the select list.

As @andrewserong notes these changes don't affect block content and don't cause any backwards compat issues, so we can easily change them again should user feedback indicate that these options are not optimal.

@andrewserong or @ramonjd are you able to retest when you have a minute and dry pushing the spacingScale.steps above 7 also to check the select list version please.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

@andrewserong or @ramonjd are you able to retest when you have a minute and dry pushing the spacingScale.steps above 7 also to check the select list version please.

That's testing nicely for me, Glen!

✅ When steps is set to 7 or lower, the numeric label is used in the post editor and global styles
✅ When steps is set to above 7, the t-shirt label is used in the select drop-down in the post editor and global styles
✅ Setting steps to 0 still disabled the spacing presets options as before
✅ In all cases saving spacing works, and the spacing appears to be rendered correctly in the site frontend both at the block level and from styles in global styles

LGTM

@ramonjd
Copy link
Member

ramonjd commented Sep 20, 2022

Before After
Screen Shot 2022-09-20 at 11 38 29 am Screen Shot 2022-09-20 at 11 38 51 am

LGTM

@ZebulanStanphill
Copy link
Member

@jasmussen I'm not convinced the t-shirt labels are even the right way to go for typography, or at least not in their current implementation. If a theme has multiple sizes considered smaller than the default, the t-shirt labels become actively misleading. See #44245. Presumably, the same idea applies to spacing presets.

@glendaviesnz glendaviesnz added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 20, 2022
@glendaviesnz
Copy link
Contributor Author

The backport of the php files will be covered by WordPress/wordpress-develop#3255

@glendaviesnz glendaviesnz merged commit 8435494 into trunk Sep 20, 2022
@glendaviesnz glendaviesnz deleted the update/spacing-presets-ui-labels branch September 20, 2022 02:13
@glendaviesnz
Copy link
Contributor Author

@jasmussen nobody else was seeing the double digits, so not sure what is going on with your local env 🤔

@github-actions github-actions bot added this to the Gutenberg 14.2 milestone Sep 20, 2022
@jasmussen
Copy link
Contributor

Thanks for landing this one. An easier way to think about it is: what if there wasn't a tooltip or a label, what notch would you be on? You'd count the number. So this feels like a better baseline to start from, and then we can always learn.

The numbers really don't work when there are more than 8 steps in the scale and it falls back to the select list:

I'd think the select-list here is the bigger challenge, and one we can revisit. Mainly, it doesn't seem like good practice for a theme to add that many presets, so ideally this should rarely if ever be seen. But if a theme does choose to do this, we could add a prefix, such as "Step 3" or "Spacing step 3" or "Padding 3", etc.

@jasmussen
Copy link
Contributor

@ZebulanStanphill for typography my personal opinion of the segmented control is that it is a curation tool. It's limited to 5 at most, and by applying the user-convenient labels it's an easy tool to use. The 5+ dropdown then becomes a more advanced tool for themes that have more specific needs than a typescale of 5. More importantly, by encouraging such a typescale, the CSS classes that get applied (such as has-small-font-size) are more likely to transfer when you switch to another theme. So long as the dropdown (which I as noted could use its own set of improvements) exists as an option for total customizability, I still lean towards the t-shirt sizes workin decently for font sizes.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Sep 20, 2022

@jasmussen Is there a way to opt into always using the dropdown, even when there's only a few options? It's just really confusing to have a type scale of "extra small -> small -> medium -> large" and have that be presented as "S -> M -> L -> XL".

If standardizing the scale is desirable for most themes, why even allow customizing the slugs/classes for that use case? It seems like the only cases that would need custom slugs/class-names are the ones that should also always use a dropdown. In other words, defining any non-standard fontSize slugs in your theme.json should automatically enable the dropdown UI, since doing so is an indication that your theme does things differently.

As it is, the current implicit yet entirely unspoken and only half-enforced standardization is confusing. I don't think an theme developer would expect the current behavior of their font size option with the slug "small" being labeled as "M" in the UI.

Alternatively (or perhaps simultaneously), it would be nice if the t-shirt labels could at least adapt when there are 2 small sizes instead of 2 large sizes. Perhaps detect the usage of an x-small slug in the first option, and use "XS -> S -> M -> ..." in that case.

@glendaviesnz
Copy link
Contributor Author

@ZebulanStanphill, @jasmussen I wonder if it is worth copying those last two comments over to #44245 so they don't get lost/forgotten ?

@ockham
Copy link
Contributor

ockham commented Sep 26, 2022

I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: fa91b9a

@ockham ockham removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 26, 2022
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 27, 2022
Package updates for bug and regression fixes:

* @wordpress/block-directory: 3.15.3
* @wordpress/block-editor: 10.0.3
* @wordpress/block-library: 7.14.3
* @wordpress/block-serialization-default-parser: 4.17.1
* @wordpress/blocks: 11.16.3
* @wordpress/components: 21.0.3
* @wordpress/compose: 5.15.2
* @wordpress/core-data: 5.0.3
* @wordpress/customize-widgets: 3.14.3
* @wordpress/edit-post: 6.14.3
* @wordpress/edit-site: 4.14.4
* @wordpress/edit-widgets: 4.14.3
* @wordpress/editor: 12.16.3
* @wordpress/format-library: 3.15.3
* @wordpress/interface: 4.16.3
* @wordpress/list-reusable-blocks: 3.15.3
* @wordpress/nux: 5.15.3
* @wordpress/preferences: 2.9.3
* @wordpress/reusable-blocks: 3.15.3
* @wordpress/server-side-render: 3.15.3
* @wordpress/style-engine: 1.0.2
* @wordpress/widgets: 2.15.3

References:
* [WordPress/gutenberg#44233 Gutenberg PR 44233] – Blocks: Fix searching of blocks when description is non-string
* [WordPress/gutenberg#44301 Gutenberg PR 44301] – Block Toolbar: update position when moving blocks
* [WordPress/gutenberg#44334 Gutenberg PR 44334] – Global Styles: Re-add styles that were removed, for classic themes
* [WordPress/gutenberg#44351 Gutenberg PR 44351] – Comments block: Support nested comments settings in the comments blocks
* [WordPress/gutenberg#44448 Gutenberg PR 44448] – Add a correct TS signature for useEntityRecords
* [WordPress/gutenberg#44315 Gutenberg PR 44315] – Pullquote: fix transform to quote crash
* [WordPress/gutenberg#44446 Gutenberg PR 44446] – Fix spacing property generation in flow layout type.
* [WordPress/gutenberg#44408 Gutenberg PR 44408] – Upgrade react-easy-crop to bring in fix for site editor iframe
* [WordPress/gutenberg#44406 Gutenberg PR 44406] – Style engine: kebab case preset slugs in the editor
* [WordPress/gutenberg#44209 Gutenberg PR 44209] – Fixing padding on the post editor when RootPaddingAwareAlignments setting is enabled
* [WordPress/gutenberg#42950 Gutenberg PR 42950] – Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter)
* [WordPress/gutenberg#44337 Gutenberg PR 44337] – Submenu block href only if url is not empty
* [WordPress/gutenberg#44291 Gutenberg PR 44291] – Add role=application to list view to prevent browse mode triggering in NVDA
* [WordPress/gutenberg#44283 Gutenberg PR 44283] – Navigation block: Fix submenu colors for imported classic menus
* [WordPress/gutenberg#44282 Gutenberg PR 44282] – Fix popover stacking in the customize widgets editor
* [WordPress/gutenberg#44247 Gutenberg PR 44247] – Spacing presets: switch to using numbers instead of t-shirt sizes for labels
* [WordPress/gutenberg#44299 Gutenberg PR 44299] – Backport template creation changes from core
* [WordPress/gutenberg#44294 Gutenberg PR 44294] – [Block Library - Query Loop]: Fix broken preview in specific category template
* [WordPress/gutenberg#44287 Gutenberg PR 44287] – [Block Library]: Rename Comments pagination inner blocks
* [WordPress/gutenberg#44256 Gutenberg PR 44256] – Avoid showing the recursion warning in previews when replacing template parts
* [WordPress/gutenberg#44265 Gutenberg PR 44265] – Blocks: officially deprecated the children and node matchers
* [WordPress/gutenberg#44251 Gutenberg PR 44251] – Global styles: Remove the beta label from global styles header

Props bernhard-reiter, cbravobernal.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54335


git-svn-id: http://core.svn.wordpress.org/trunk@53894 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Sep 27, 2022
Package updates for bug and regression fixes:

* @wordpress/block-directory: 3.15.3
* @wordpress/block-editor: 10.0.3
* @wordpress/block-library: 7.14.3
* @wordpress/block-serialization-default-parser: 4.17.1
* @wordpress/blocks: 11.16.3
* @wordpress/components: 21.0.3
* @wordpress/compose: 5.15.2
* @wordpress/core-data: 5.0.3
* @wordpress/customize-widgets: 3.14.3
* @wordpress/edit-post: 6.14.3
* @wordpress/edit-site: 4.14.4
* @wordpress/edit-widgets: 4.14.3
* @wordpress/editor: 12.16.3
* @wordpress/format-library: 3.15.3
* @wordpress/interface: 4.16.3
* @wordpress/list-reusable-blocks: 3.15.3
* @wordpress/nux: 5.15.3
* @wordpress/preferences: 2.9.3
* @wordpress/reusable-blocks: 3.15.3
* @wordpress/server-side-render: 3.15.3
* @wordpress/style-engine: 1.0.2
* @wordpress/widgets: 2.15.3

References:
* [WordPress/gutenberg#44233 Gutenberg PR 44233] – Blocks: Fix searching of blocks when description is non-string
* [WordPress/gutenberg#44301 Gutenberg PR 44301] – Block Toolbar: update position when moving blocks
* [WordPress/gutenberg#44334 Gutenberg PR 44334] – Global Styles: Re-add styles that were removed, for classic themes
* [WordPress/gutenberg#44351 Gutenberg PR 44351] – Comments block: Support nested comments settings in the comments blocks
* [WordPress/gutenberg#44448 Gutenberg PR 44448] – Add a correct TS signature for useEntityRecords
* [WordPress/gutenberg#44315 Gutenberg PR 44315] – Pullquote: fix transform to quote crash
* [WordPress/gutenberg#44446 Gutenberg PR 44446] – Fix spacing property generation in flow layout type.
* [WordPress/gutenberg#44408 Gutenberg PR 44408] – Upgrade react-easy-crop to bring in fix for site editor iframe
* [WordPress/gutenberg#44406 Gutenberg PR 44406] – Style engine: kebab case preset slugs in the editor
* [WordPress/gutenberg#44209 Gutenberg PR 44209] – Fixing padding on the post editor when RootPaddingAwareAlignments setting is enabled
* [WordPress/gutenberg#42950 Gutenberg PR 42950] – Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter)
* [WordPress/gutenberg#44337 Gutenberg PR 44337] – Submenu block href only if url is not empty
* [WordPress/gutenberg#44291 Gutenberg PR 44291] – Add role=application to list view to prevent browse mode triggering in NVDA
* [WordPress/gutenberg#44283 Gutenberg PR 44283] – Navigation block: Fix submenu colors for imported classic menus
* [WordPress/gutenberg#44282 Gutenberg PR 44282] – Fix popover stacking in the customize widgets editor
* [WordPress/gutenberg#44247 Gutenberg PR 44247] – Spacing presets: switch to using numbers instead of t-shirt sizes for labels
* [WordPress/gutenberg#44299 Gutenberg PR 44299] – Backport template creation changes from core
* [WordPress/gutenberg#44294 Gutenberg PR 44294] – [Block Library - Query Loop]: Fix broken preview in specific category template
* [WordPress/gutenberg#44287 Gutenberg PR 44287] – [Block Library]: Rename Comments pagination inner blocks
* [WordPress/gutenberg#44256 Gutenberg PR 44256] – Avoid showing the recursion warning in previews when replacing template parts
* [WordPress/gutenberg#44265 Gutenberg PR 44265] – Blocks: officially deprecated the children and node matchers
* [WordPress/gutenberg#44251 Gutenberg PR 44251] – Global styles: Remove the beta label from global styles header

Props bernhard-reiter, cbravobernal.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54335


git-svn-id: https://core.svn.wordpress.org/trunk@53894 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
Package updates for bug and regression fixes:

* @wordpress/block-directory: 3.15.3
* @wordpress/block-editor: 10.0.3
* @wordpress/block-library: 7.14.3
* @wordpress/block-serialization-default-parser: 4.17.1
* @wordpress/blocks: 11.16.3
* @wordpress/components: 21.0.3
* @wordpress/compose: 5.15.2
* @wordpress/core-data: 5.0.3
* @wordpress/customize-widgets: 3.14.3
* @wordpress/edit-post: 6.14.3
* @wordpress/edit-site: 4.14.4
* @wordpress/edit-widgets: 4.14.3
* @wordpress/editor: 12.16.3
* @wordpress/format-library: 3.15.3
* @wordpress/interface: 4.16.3
* @wordpress/list-reusable-blocks: 3.15.3
* @wordpress/nux: 5.15.3
* @wordpress/preferences: 2.9.3
* @wordpress/reusable-blocks: 3.15.3
* @wordpress/server-side-render: 3.15.3
* @wordpress/style-engine: 1.0.2
* @wordpress/widgets: 2.15.3

References:
* [WordPress/gutenberg#44233 Gutenberg PR 44233] – Blocks: Fix searching of blocks when description is non-string
* [WordPress/gutenberg#44301 Gutenberg PR 44301] – Block Toolbar: update position when moving blocks
* [WordPress/gutenberg#44334 Gutenberg PR 44334] – Global Styles: Re-add styles that were removed, for classic themes
* [WordPress/gutenberg#44351 Gutenberg PR 44351] – Comments block: Support nested comments settings in the comments blocks
* [WordPress/gutenberg#44448 Gutenberg PR 44448] – Add a correct TS signature for useEntityRecords
* [WordPress/gutenberg#44315 Gutenberg PR 44315] – Pullquote: fix transform to quote crash
* [WordPress/gutenberg#44446 Gutenberg PR 44446] – Fix spacing property generation in flow layout type.
* [WordPress/gutenberg#44408 Gutenberg PR 44408] – Upgrade react-easy-crop to bring in fix for site editor iframe
* [WordPress/gutenberg#44406 Gutenberg PR 44406] – Style engine: kebab case preset slugs in the editor
* [WordPress/gutenberg#44209 Gutenberg PR 44209] – Fixing padding on the post editor when RootPaddingAwareAlignments setting is enabled
* [WordPress/gutenberg#42950 Gutenberg PR 42950] – Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter)
* [WordPress/gutenberg#44337 Gutenberg PR 44337] – Submenu block href only if url is not empty
* [WordPress/gutenberg#44291 Gutenberg PR 44291] – Add role=application to list view to prevent browse mode triggering in NVDA
* [WordPress/gutenberg#44283 Gutenberg PR 44283] – Navigation block: Fix submenu colors for imported classic menus
* [WordPress/gutenberg#44282 Gutenberg PR 44282] – Fix popover stacking in the customize widgets editor
* [WordPress/gutenberg#44247 Gutenberg PR 44247] – Spacing presets: switch to using numbers instead of t-shirt sizes for labels
* [WordPress/gutenberg#44299 Gutenberg PR 44299] – Backport template creation changes from core
* [WordPress/gutenberg#44294 Gutenberg PR 44294] – [Block Library - Query Loop]: Fix broken preview in specific category template
* [WordPress/gutenberg#44287 Gutenberg PR 44287] – [Block Library]: Rename Comments pagination inner blocks
* [WordPress/gutenberg#44256 Gutenberg PR 44256] – Avoid showing the recursion warning in previews when replacing template parts
* [WordPress/gutenberg#44265 Gutenberg PR 44265] – Blocks: officially deprecated the children and node matchers
* [WordPress/gutenberg#44251 Gutenberg PR 44251] – Global styles: Remove the beta label from global styles header

Props bernhard-reiter, cbravobernal.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54335 602fd350-edb4-49c9-b593-d223f7449a82
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs User Documentation Needs new user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants