Add support for non-integer font size hints in the Font Size Picker component.

[ndiego]

Description

The Font Size Picker component displays hints which alert the user to the numerical value of the font size. For example a registered font size of Large with a size of 32px, should show a "32" in the font size menu and the hint in the header would display "32px" when Large is chosen.

Currently, this setup does not work for values that are not integers. For example 1.25rem does not display any hints whatsoever. This PR fixes that and adds support for font sizes that include decimal points, much like those provided in the upcoming Twenty Twenty-Two theme.

Further PRs can explore how to handle values that include var( ... ), clamp( ... ), etc. As before, no hints are provided for font sizes with such values.

How has this been tested?

Screenshots

Twenty Twenty-Two theme without fix.

Twenty Twenty-Two theme with fix. (note the Large and Huge sizes use clamp())

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[ndiego]

@ntsekouras I apologize for the ping, I know you have a ton on your plate. But I would love to see if these could get merged for 5.9. Without it, the font-size hints in Twenty Twenty-Two are missing since the theme uses rem font sizes with decimal points. It's a small thing, but would improve the UI for people exploring the new theme. That said, I understand if this needs to wait.

[ciampo]

Thank you @ndiego for working on this!

I wonder if we should add any unit tests? They would make sure that we can cover all the edge-case scenarios that can come to mind.

I'm just not very knowledgeable about what the expected behaviour of the font-size picker should be when displaying these "hints".

I think the average user is used to see a value in px (e.g 32), but would they feel confused if they instead saw 1.25?

Also, is there the possibility that different font sizes are defined with different units (e.g. 1.25rem, 2vh)? If in that case the hints displayed 1.25 and 2, that would be even more confusing, since those numbers don't share the same unit.

Anyway, these were just thoughts — I think @ntsekouras knows better the context around the font size picker. Also cc'ing @jasmussen, since I believe his opinion here could be valuable.

[ciampo]

If I understand correctly the regex, if size doesn't contain any numeric value at all, numericValue won't actually be a numeric value, e.g.

splitValueAndUnitFromSize('var(--some-css-variable)') // ['var(--some-css-variable)', undefined]

[ndiego]

You are correct, this was intended as a quick fix so that the hints would not "break" when decimal point values were used as they are in Twenty Twenty-Two. Ultimately a full overhaul of how these hints are generated is warranted. It does get very complicated once the theme creator uses a variety of different units. I am not sure we would need to account for everything as some onus needs to be on the theme developer. 🤔

[ntsekouras]

I haven't checked the code thoroughly, but I'm wondering why you changed the used regex function instead of just the regex.

[jasmussen]

Thanks so much for the PR!

I think there's a core of a problem with how our design tools currently handle "nonstandard" values, exactly as you suggest with min/max/clamp. I believe padding inputs that get these values from theme.json or patterns, for example, show up as empty. So there's a challenge with how we surface this, which seems important to solve on the input-field-level.

For the font size selector, though, I can't help but echo Marco, though:

I think the average user is used to see a value in px (e.g 32), but would they feel confused if they instead saw 1.25?

Considering the dropdown is specifically created to provide some ergonomic shortcuts to theme-appropriate sizes, to me the descriptions inside the dropdown (Small, Medium, etc) feel far more valuable than any hints. As single numbers representing pixel values, I can see how those surfaced as subtle hints can add a little value as well, since they represent an obvious increase in sizes, whereas a value like 1.25 (even if we included the unit, 1.25rem) would be hard to parse for anyone that isn't a web developer.

In that vein, I'd be tempted to think the current behavior is intentional: allow any value, but only surface the number hints when they are ergonomic.

This is not a strong opionion, but I think as an alternative we should remove all the hints, even for the pixel values.

[ndiego]

@jasmussen I completely agree. However I stumbled upon this while testing Twenty Twenty-Two. Since they are used rem and the smallest font size is 1rem, the current setup displays the 1. I was confused why there was a single value and all else were empty, hence the PR. But I could see just removing these hints. 🤔

[ciampo]

I also personally think that removing the hints completely until we figure out a potentially alternative solution may be the better thing to do here

[ntsekouras]

Thanks for the PR @ndiego and sorry for being late in the discussions, but I'm still AFK and will look at it better from Monday.

My first thoughts though are that while handling the decimals case should be added, I'm not sure about removing the hints entirely. This is because units are something that themes control (setting these values) and with the current update of FontsizePicker this seems just like a bandaid because if the theme has less than 6 values, the hint in the SelectControl is the actual label of the SegmentedControl. If we are not sure about the shown hint/labels when other units are used, we should probably rethink the whole control. What do you think?

[ndiego]

No worries @ntsekouras! @aaronrobertshaw has iterated on this PR with his own, #36636. I recommend we go with that one for 5.9.

[ndiego]

Closing this in favor of #36636, which was recently merged.

[ntsekouras]

Thanks for all your work Nick!

[jasmussen]

Yes, thank you!