[Components - FontSizePicker]: Use incremental sequence of numbers as labels for the available font-sizes at the segmented control (conditionally) #37038
Conversation
ntsekouras
requested review from
youknowriad,
mtias,
mirka,
oandregal,
ciampo,
kjellr,
jasmussen,
diegohaz,
jorgefilipecosta,
pablohoneyhoney and
aaronrobertshaw
|
Size Change: +56 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
Nice! Do we want to provide a default alias for the font sizes declared in Gutenberg's (and core's) |
If we go with the approach to use it only when complex css values are available (see PR's description), there is no need to add |
Oh, interesting. If the design intention is to show numbers in the segmented control, what if we only allowed numbers in the alias field? The advantage would be that the data the users see is always consistent (numbers) no matter how the theme is set up internally. |
There was a problem hiding this comment.
Hey @ntsekouras , thank you for opening this PR!
- Should we add some unit tests and a Storybook example for this scenario?
- Would you be able to also add a CHANGELOG entry?
Oh, interesting. If the design intention is to show numbers in the segmented control, what if we only allowed numbers in the alias field? The advantage would be that the data the users see is always consistent (numbers) no matter how the theme is set up internally.
I think that allowing numbers as aliases would create confusion, since the main reason why we need these aliases in the first place is because they need to represent "dynamic" values.
| const fontSizesContainComplexValues = fontSizes.some( | ||
| ( { size } ) => ! isSimpleCssValue( size ) | ||
| ); | ||
| const allFontSizesHaveAliases = fontSizes.every( |
There was a problem hiding this comment.
Just a though — I wonder if the decision to display an alias should be made for each "font size" independently — i.e.:
- There are more than 6 presets? Display a select control
- Less than 6 presets:
- Display a select control only if at least one font size is not a simple control, and doesn't have an alias either
- Otherwise, display a toggle group
This mean that, as far as all "complex" values are paired with an alias, we should be able to show a toggle group
There was a problem hiding this comment.
Marco's suggestion here matches the preconceived notion I had of how this would work before I read the PR description.
My thinking was that the alias would act as an override or fallback. Something for theme authors to provide when we can't determine a sensible value from the font size.
I can also see the other angle where this could lead to an inconsistent UI. A theme author adds aliases such as XL for complex values but omits aliases for simple ones, resulting in a mixture of numeric and string labels in the segmented control. A similar situation could arise if we wished to use the aliases for hints within the select control.
If we made the decision to display aliases at the individual font size level, it might alleviate the need to plumb through shouldUseAliases.
There was a problem hiding this comment.
I wonder if the decision to display an alias should be made for each "font size" independently
I agree with Aaron:
where this could lead to an inconsistent UI. A theme author adds aliases such as XL for complex values but omits aliases for simple ones, resulting in a mixture of numeric and string labels in the segmented control.
| const shouldUseAliases = | ||
| fontSizesContainComplexValues && allFontSizesHaveAliases; |
There was a problem hiding this comment.
What happens if a font size is a "simple" value, but has an associated alias? Should the alias take the precedence?
There was a problem hiding this comment.
Based on the initial design with the intention of showing numeric values, I'd say no..
There was a problem hiding this comment.
Very nice @ntsekouras ✨
This tests as advertised for me in both the Storybook and Block Editor. The knobs in the story to automate updating the font size configuration are a nice touch, thanks 👍.
In general, the alias approach looks like a good step forward and might only need a few tweaks as proposed by others.
| @@ -65,10 +65,11 @@ If no value exists, this prop defines the starting position for the font size pi | |||
|
|
|||
| ### fontSizes | |||
|
|
|||
| An array of font size objects. The object should contain properties size, name, and slug. | |||
| An array of font size objects. The object should contain properties `size`, `name`, `slug` and can contain `alias` on special use cases described right below. | |||
There was a problem hiding this comment.
Feel free to ignore this suggestion. It's my attempt to make this read a little better, so it's completely subjective.
| An array of font size objects. The object should contain properties `size`, `name`, `slug` and can contain `alias` on special use cases described right below. | |
| An array of font size objects. Each object should contain the properties `size`, `name`, and `slug`. Optionally, `alias` can be included for special use cases as described below. |
| const fontSizesContainComplexValues = fontSizes.some( | ||
| ( { size } ) => ! isSimpleCssValue( size ) | ||
| ); | ||
| const allFontSizesHaveAliases = fontSizes.every( |
There was a problem hiding this comment.
Marco's suggestion here matches the preconceived notion I had of how this would work before I read the PR description.
My thinking was that the alias would act as an override or fallback. Something for theme authors to provide when we can't determine a sensible value from the font size.
I can also see the other angle where this could lead to an inconsistent UI. A theme author adds aliases such as XL for complex values but omits aliases for simple ones, resulting in a mixture of numeric and string labels in the segmented control. A similar situation could arise if we wished to use the aliases for hints within the select control.
If we made the decision to display aliases at the individual font size level, it might alleviate the need to plumb through shouldUseAliases.
|
I'm preparing the corresponding PR for wp-cli (i18n command) at wp-cli/i18n-command#292 |
I see. I'm still trying to wrap my head around the design direction for this component, so bear with me :) The thought I'm struggling with is that this component displays data differently depending on the theme's internal implementation: sometimes numbers and sometimes letters. It'd be great if we could find a way to show the same type of data. Of course, not a blocker for this PR. |
|
If we can land this one quick and then polish the details it'd be helpful to help us land the corresponding wp-cli PR wp-cli/i18n-command#292 (we shouldn't merge that one until this direction is confirmed and merged). |
|
Thanks for the PR! I'll share some more high level thoughts, that can hopefully be helpful. What we want to avoid is this:
With only 5 sizes defined, a segmented control would be a much more immediate and convenient control. Going back to this design and this clarification, it seems like the best path forward is:
2 is where it gets difficult. Here's what TT2 offers: On the one hand TT2 might be bordering on too clever with the clamp based values, at the same time: themes can do this, so they will. But we still can't extract numbers from that, at best we'd end up here: Those aren't helpful indicators, and might even wrap onto multiple lines. Question: can we try to force T-shirt sizes, S/M/L/XL/XXL, in sequence? First defined value is "S", 5th define value is "XXL"? Adding the explicit alias feels like letting the genie out of the bottle, allowing theme authors to add long aliases that cause a segmented control to wrap. Ultimately it might be what we need to do, but it would be nice to first see if the predefined values could work. What do you think? |
|
Thanks all the reviews and help here! ❤️
@ciampo there is a story for this. Regarding the unit tests, did you have something specific in mind like for taking a snapshot of the rendered html?
That will be added yes.
Probably no, because even if this should be the right declaration order (smaller->bigger) this is up to the theme and that order is not guaranteed.
Actually there could be a simpler alternative, not so flexible but then again this PR accommodates more complex use cases from theme developers, who know what they are doing, and also handles the
Pros:
Cons:
I think it's probably better to loose the What do you think? |
So if I had Small/Normal/Medium/Large/Huge it'd be something like this? While I like the idea of starting with simple automation before adding any additional JSON properties, I can't help but feel like those letters don't translate well to sizes in the same way the T-shirt labels do. I understand that a fixed sequence isn't going to be accurate if a developer decides to start with Large as the first one, or even mix and match. That that doesn't seem like a use case to optimize for. This is not a very strong opinion, but to keep things simple, I'd start with numbers and t-shirt sizes, and then revisit the t-shirt sizes if the fixed sequence turns out to be a problem in practice. |
|
For the case of Twenty Twenty-Two, we can change those names to something else if necessary — we were just borrowing the names from Gutenberg when we named those. |
ntsekouras
force-pushed
the
try/add-font-sizes-alias
branch
from
67558bb
to
48fefb5
Compare
Sure, I'll open that issue today. Edit: Done here: #37378 |
|
Got an initial PR to update the default font sizes at #37381 I'd appreciate heavy testing to make sure that by doing this, we don't introduce any regressions for normal & huge classes/variables. Edit: also, I presume we want to update the default font-sizes in 5.9? |
noisysocks
added
the
[Priority] High
ntsekouras
force-pushed
the
try/add-font-sizes-alias
branch
from
be498f4
to
897948d
Compare
|
I just rebased and with a green CI, I'm going to merge this. The next PR to be merged is the one that changes the core presets and that will only leaves a PR that I'll start today for the below:
Noting that we will also need the PR from @kjellr to change TwentyTwentyTwo |
ntsekouras
changed the title
T-shirt aliases to segmented control (conditionally)
|
Thanks for all the work here, @ntsekouras ! |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
… labels for the available font-sizes at the segmented control (conditionally) (#37038) * [Components - FontSizePicker]: Add alias option in font sizes * use `T-shirt` sizes * add changelog entry * add unit tests * use incremental sequence of numbers as labels
… labels for the available font-sizes at the segmented control (conditionally) (#37038) * [Components - FontSizePicker]: Add alias option in font sizes * use `T-shirt` sizes * add changelog entry * add unit tests * use incremental sequence of numbers as labels
The problem with this is that if themes don't follow a standard naming convention, when users switch themes, the additional font-sizes will break. I think it would be good to standardize on a naming convention all themes can use so that if themes do want to add more they will be persisted across themes. |
Wasn't this the case before too? I guess it's the same thing with theme's presets of colors, etc.., no? |
|
Yes, it was the case before this change and is also an issue with colors. |
Part of #36545
Currently the logic for showing the segmented control depends on having at most five font sizes and only if none of them contain css complex values (
clamp, var, etc..). This is because on the segmented control we show as labels the actual numeric value of the font size and it's not possible to calculate these dynamic values.To accommodate use cases where at most five font sizes are available but also contain complex css values, this PR proposes to use an incremental sequence of numbers (1/2/3/4/5) as labels for the available font-sizes automatically. As @jasmussen noted:
The previous approach with adding an
aliasproperty to font size object is also a convention that could create more confusion to theme developers.Screen.Recording.2021-12-15.at.10.07.48.AM.mov
Testing instructions
Storybook (
npm run storybook:dev)Change values through the available knobs in
With Complex Css ValuesstoryWith a block theme
fontSizesat least one complex css value