Add shortName property to FontSize Preset to override t-shirt size label

[fabiankaegy]

What?

Add a new shortName property which can optionally be added to any FontSize Preset in theme.json which gets used instead of hardcoded t-shirt size in FontSizePicker

Why?

As outlined in #44245 there are several usecases where font sizes may not be following the t-shirt size scale. When you for example start with XS and make your way from there. Or have multiple small variants you want to be able to better control the labels that get used for the various custom font size options.

Closing: #44245

How?

By adding a new optional shortName property to font size presets.

{
    "apiVersion": 2,
    "settings": {
        "typography": {
            "fontSizes": [
                {
                    "slug": "xs",
                    "name": "Extra Small",
                    "value": "10",
                    "shortName": "XS"
                }
            ]
        }
    }
}

Testing Instructions

• Define at least one font size preset that has a shortName defined in it's theme.json.
• Verify that the label renders correctly in the FontSizePicker

Testing Instructions for Keyboard

Screenshots or screencast

[fabiankaegy]

For my local testing I updated the emptytheme so that the theme.json file looks like this:

{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		},
		"typography": {
			"fontSizes": [
				{
					"name": "XSmall",
					"size": "12px",
					"slug": "xsmall",
					"shortName": "XS"
				},
				{
					"name": "Custom",
					"size": "12px",
					"slug": "custom",
					"shortName": "CC"
				},
				{
					"name": "Large",
					"size": "12px",
					"slug": "large"
				}
			]
		}
	},
	"patterns": [ "short-text-surrounded-by-round-images", "partner-logos" ]
}

[Mamaduka]

Suggested change

	fontSize.shortName &&
	hasValidShortName( fontSize.shortName )
	hasValidShortName( fontSize?.shortName )

Suggestion: The hasValidShortName will return false if short name is undefined.

[fabiankaegy]

@Mamaduka the reason I added back in the duplicate check is that TypeScript is complaining about it for whatever reason... See https://github.com/WordPress/gutenberg/actions/runs/3706287491/jobs/6281315087

[Mamaduka]

I just noticed it. Sorry about that. Let's revert for now, and maybe more TS-savvy people can help us here.

[fabiankaegy]

Reverted in dc9fdd4

[noisysocks]

It should work to remove this so long as hasValidShortName's argument type is string | undefined.

[fabiankaegy]

Yeah for whatever reason that doesn't work for me. As per your later comment, I now removed the | undefined type from the function and kept the duplicate check inline.

[Mamaduka]

I'm unfamiliar with this component, but the code looks good to me 👍

Probably @noisysocks or @ramonjd can provide better feedback here.

P.S. I think it's a missed opportunity not to call the new property the shirtName 😄

[t-hamano]

Thanks for the great PR, @fabiankaegy !

I have confirmed that it generally works correctly. It works correctly with add_theme_support( 'editor-font-sizes') as well as theme.json.

39317f4f9a6d7be2098a530f49eef0cd.mp4

I think the idea of short names of 3 characters or less being considered correct is great, but it may cause text wrapping in non-English languages.

I think this is probably because the width of a single letter varies from language to language.

One possible approach is to either allow only alphabetical characters or reduce the left and right padding.
What do you think?

[t-hamano]

@jasmussen
If you don't mind, I would like to get some design feedback on this PR 🙏

[fabiankaegy]

@t-hamano Thanks for your detailed testing! I was concerned about non-Latin characters and how they would relate to the t-shirt sizing.

I'm curious, how does the UI currently treat the t-shirt sizes? Are they not translated and shown as XL etc for all languages?

[noisysocks]

Is this check necessary? Is it possible to pass e.g. a number to shortName? I thought theme.json was validated against the schema but could be wrong.

[ramonjd]

I can't see any coercion going on in the background. In fact, without the type check it'll throw errors if shortName is 0 or undefined:

So it seems good to leave it as is?

[noisysocks]

Makes sense. cc. @glendaviesnz as he added the original T-shirt size feature.

[glendaviesnz]

Makes sense. cc. @glendaviesnz as he added the original T-shirt size feature.

I think it was @ramonjd actually, and I copied it for spacing sizes, but it was subsequently removed from there.

[ramonjd]

Also think this is a good move. I smoke tested:

shortName in theme.json takes precedence and the fallback display where there are no values.

Select is unaffected.

👍

[t-hamano]

@fabiankaegy

I'm curious, how does the UI currently treat the t-shirt sizes? Are they not translated and shown as XL etc for all languages?

I tested how it translates in CJKV (Chinese, Japanese, Korean, Vietnam) languages.
T-shirt sizes are commonly used in Asian countries, but only Japanese is localized.

Language	Screenshot
Chinese
Japanese
Korean
Vietnam

When the number of variations was five, multibyte characters seem to be wrapped even with two characters.

However, since unintentional text wrapping caused by localization is not limited to this component, improvements in this area might be considered as a follow-up.

[jasmussen]

Hey folks, catching up! I really appreciate the attention to detail and motivation for flexibility that is driving the work here, and seeing the collaboration is inspiring.

I would, however, caution that when we add a new API like this, we have to be extra careful, and increase our confidence more than usual, before we land it. It's orders of magnitude easier to add an API than it is to remove one.

In this case, there are a couple of flags that suggest we need a bit of a wider take, to boost that confidence (CC: @WordPress/gutenberg-core):

• The segmented control can hold 5 values, but if any of those values are more than 1 or 2 glyphs, the control itself explodes and breaks.
• In terms of short names, we've tried numbers, we're trying T-shirt sizes, and I'm somewhat struggling to think of other appropriate terms to put in there. What examples can you think of? In general it would seem the primary use case is to allow, for example, multiple small variants, or multiple large variants, but it seems like this might be better served with an API that turns off the segmented control entirely and forces the dropdown, which can provide additional context.
• When switching themes or even style variations within one theme, the difference in sizes needs extra careful thought so that values you saved in one somewhat carry over to the other. The forced S-XL (or 1-5 if we went back to numbers) carries that value implicitly by not being configurable. It kind of implies that these 5 base values, be mindful when you define them, so a theme has good transferrable properties between themes.
• There's been skepticism about the t-shirt size interface itself, which could indicate it needs an entirely different approach

As is, my personal take is that the intent behind this PR is right (thank you), but the execution of the API is likely to have a number of side effects and ultimately might lead to bad experiences. In that light, it might be more useful to enable the dropdown with a prop change. What do you think?

[LuigiPulcini]

Wouldn't it be more appropriate to call the new property label instead of shortName, for consistency with the ToggleGroupControl component?

gutenberg/packages/components/src/font-size-picker/font-size-picker-toggle-group.tsx

Lines 32 to 33 in 30a4d86

	label={ T_SHIRT_ABBREVIATIONS[ index ] }
	aria-label={ fontSize.name || T_SHIRT_NAMES[ index ] }

L32 could be easily transformed into the following, which is consistent with what L33 already does:

label={ fontSize.label || T_SHIRT_ABBREVIATIONS[ index ] }
aria-label={ fontSize.name || T_SHIRT_NAMES[ index ] }

once the new label property is also added as an optional property of the FontSize type.

This naming would make it more intuitive.

[fabiankaegy]

Closing this PR in favor of another solution that can be used to force the dropdown. Thanks for all the discussion :)