Font Size Picker: Allow non-integers as simple CSS values and in hints #36636
Conversation
aaronrobertshaw
added
[Feature] UI Components
|
Size Change: +2.61 kB (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you @aaronrobertshaw for iterating on the approach taken in #36420 ! Having unit tests for isSimpleCssValue and splitValueAndUnitFromSize gives us confidence and also doubles as live documentation of how we expect these functions to work.
Changes LGTM, and they test well as per the instructions in both Storybook and the Block Editor.
Could you also add a CHANGELOG entry in the components package before merging?
| */ | ||
| const [ , numericValue, unit ] = size.split( /(\d+)/ ); | ||
| return [ numericValue, unit ]; | ||
| const [ numericValue, unit ] = `${ size }`.match( /[\d\.]+|\D+/g ); |
There was a problem hiding this comment.
Looking at the changes made in #36420, I believe that we should also apply the change on the JSDoc for the size param (i.e. changing its type to be only string)
There was a problem hiding this comment.
The changes here were slightly different to #36420. It honours what was documented in terms of the param accepting string|number. The tests also cover both strings and numbers.
The FontSizePicker docs themselves also include numerical size values. Should we not maintain compatibility with that?
In light of that, do you still believe I should restrict this?
There was a problem hiding this comment.
You actually made a few good points — let's leave it as-is (and thank you for looking into it)
There was a problem hiding this comment.
I was wondering why you changed from split to match. I was expecting a change in the regex.
There was a problem hiding this comment.
Honestly, this wasn't a conscious choice. It came from when I copied the original change.
On a closer look though, match appears a little cleaner in terms of its results. I don't feel strongly about it, so if there is a good reason to change it back to split I can.
|
This looks great, thanks for the iteration @aaronrobertshaw 🙌 |
|
Also, if not already planned, I feel this should be backported to 5.9, especially since Beta 1 has been delayed. |
|
Before landing this PR, let's just re-read #34345 a bit, and perhaps get a sanity check from @pablohoneyhoney. It looks like the hints might be intentional, but with some unit specific design to follow. |
|
Like @jasmussen I'd also wanted to advise for sanity check about this. I also commented in the other PR from @ndiego
|
I think I'm lacking context and history on this as I'm not following what the issue is.
The only hints that were removed were those from the
There is a separate PR adding tooltips to the the Example font sizes array used in demos below
If we are to rethink the whole control. I'd propose we still pursue these refinements as an interim step forward.
The mockup included in #34345 doesn't appear to match what is currently on trunk either and looks pretty pixel-centric. The component as is on trunk only spits out the font size option's
The checklist of tasks also has the following item:
Given the wrinkles found to date, I'm not sure we can consider the design final either. I think the changes in this PR help provide some more solid middle ground while we refine what this component should ultimately be. |
|
Thanks for the comment, Aaron, and thanks for all the explorations. In trying out the various approaches, and revisiting the mockups, even though the hints are pixel centric, they do come up again and again and in doing so feel more important to the design than I gave credit in my past comment. In light of those comments it seems like we might want to allow the non-integers but still keep the hints after all. If it turns problematic, we can remove them in the future. I realize that circles us a bit back to @ndiego's original PR! Sorry about that everyone. What do you think? |
I could only see the two mockups on the Typography Tools: Tracking issue, do you have others? To play devil's advocate here, does just because the hints are in the mockups mean they have to be or should be? The mockups don't cover the case of mixed units so appear incomplete to me or needing more thought.
We've already seen issues with simple vs complex CSS values, integers vs non-integers etc. It feels like we are sticking our head in the sand a little here. I'll go with whatever the majority view is though. Adding back the labels we can get the following example where the hints don't appear to make sense and could add confusion rather than alleviate it. In the example below a user might think "Why on earth is Small '16' when Extra Large is '3.5'?" I appreciate a theme author is unlikely to mix
Example font size config used in demo above
I believe we should persist with this PR as it adds unit tests, improved header hint for complex/dynamic CSS values etc. |
The ones I looked at are these, which come from the Global Styles figma.
Oh mockups are never perfect, hence a bit of my reluctance in the first place. They're always a kind of glimpse into a potential future, and these implementation bouts that make it real surface key information as to which way to go. At the end of the day, this one is on me: my first instinct was to remove the hints entirely since they couldn't work for all cases. But in marinating on the mockups a bit more, trying to understand the thought behind them, it's just feeling clear to me that the hints are there as a numeric addition to the phrases, Tiny, Small, etc. Even if 1, 1.5, 2 (referring to rem units) might not be as obvious of a type scale as 12, 14, 18, 24 etc — it does still show a progression and delta. Sorry about the back and forth!
Yep, that could be an argument for adding the units in the future. I want to think this is an edgecase, and that most themes will stick to a single unit. But this is definitely something we can come back to. Thanks again for this one. |
I would also agree here that this would be an edgecase. Also I'm not sure if showing |
It's fair to design for the most common case, and not optimize for an edgecase of bad practice. Because the numeric values do provide more precision to concepts that might be understood subjectively by different people. Semantics like Small can be interpreted differently by different users with diverse knowledge, design approaches, biases, and backgrounds. |
aaronrobertshaw
changed the title
Fair point. I've removed the The PR description and title have been updated for the current state of this PR. |
There was a problem hiding this comment.
Thanks for iteration and the tests @aaronrobertshaw ! 💯
Let's ship to handle the decimals for now.
|
Was out for the past couple days. This looks great! 🙌 |
#36636) * Remove hints and allow non-integers * Reinstate hints * Remove Dynamic label from header hints
Related:
Description
This PR:
1.5remto be accepted as simple CSS values0.75emisSimpleCssValueandsplitValueAndUnitFromSizeutil functionsNote: This PR builds upon the approach taken and discussion in #36420. Credit goes to @ndiego
Earlier iterations of this PR removed the hints from the select control however upon further discussion they have been reinstated.
How has this been tested?
Unit Tests:
npm run test-unit:watch packages/components/src/font-size-picker/testutils.jstestsStorybook:
npm run storybook:devBlock Editor:
Screenshots
Types of changes
Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).