Update design of FontSizePicker when withSlider is set #44598
Conversation
noisysocks
added
[Type] Enhancement
|
Size Change: -67 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
| @@ -19,6 +19,7 @@ | |||
| margin-bottom: $grid-unit-20; | |||
| background: $gray-100; | |||
| border-radius: $radius-block-ui; | |||
| overflow: hidden; | |||
There was a problem hiding this comment.
This stops the Aa text breaking out of the font preview container when the font size is really large.
| isFinite( Number( numericValue ) ) | ||
| ) { | ||
| return [ numericValue, unit ]; | ||
| export function parseNumberAndUnitFromSize( |
There was a problem hiding this comment.
Not really necessary for this PR but I thought I'd update this function to:
- accept
undefined, since that's a valid input toFontSizePicker; - be more strict about what is returned, i.e.
- specify a return type;
- return a
numberinstead of astringcontaining a number;
- use a regular regex with capturing groups instead of relying on
/gbehaviour; - not use "value" in the name since that can be confused with the
valueprop inFontSizePicker.
There was a problem hiding this comment.
UnitControl has got a bunch of utils function that potentially achieve a very similar (if not the same) result: https://github.com/WordPress/gutenberg/blob/1326a46cf10ad54e897d1f3c9aaa0e385aba896a/packages/components/src/unit-control/utils.ts
Maybe we could use those function directly? It would remove redundant code AND make sure that we're more consistent on the way we parse unit and values.
Also happy if we want to leave this task separate and work on it in the future.
There was a problem hiding this comment.
Oh fantastic! That lets us delete this altogether 😀
| const [ , firstFontSizeUnit ] = parseNumberAndUnitFromSize( | ||
| fontSizes[ 0 ]?.size | ||
| ); | ||
| const hasUnits = !! valueUnit || !! firstFontSizeUnit; |
There was a problem hiding this comment.
Also not really necessary for this PR but was irking me. Previously hasUnits was calculated by checking the type of value and the first option's size. If either was a string then we'd allow units. This isn't 100% correct though since technically a font size can be a string containing a number with no unit, i.e. "30". This new logic uses parseNumberAndUnitFromSize which is a little more tedious but correct.
There was a problem hiding this comment.
I was wrong about this. See #44598 (comment).
| }: { | ||
| selectedItem: FontSizeSelectOption; | ||
| } ) => { | ||
| if ( selectedItem.size === undefined ) { |
There was a problem hiding this comment.
I changed all of the onChange callbacks to explicitly handle the undefined case because:
- I think it's easier to understand, and;
- if
hasUnitsisfalsethen you could get into a situation whereonChangeis called withNaNbecauseNumber( undefined ) === NaN.
| min={ isValueUnitRelative ? 0.75 : 12 } | ||
| max={ isValueUnitRelative ? 6.25 : 100 } | ||
| step={ isValueUnitRelative ? 0.05 : 1 } |
There was a problem hiding this comment.
I chose these new em/rem values to be roughly equivalent to the previous px values assuming a 16px base font size.
There was a problem hiding this comment.
These values feel nice to play with in the UI! 👍
There was a problem hiding this comment.
@glendaviesnz also made similar changes in #44959 for spacing. Font size is a bit different to spacing, so not sure if it makes sense to align these values.
There was a problem hiding this comment.
Nice work @noisysocks, this is looking really good! Just left a couple of questions, but no real issues from the code side of things to me 👍
From the UI perspective, these are all mostly nits / fine-tuning notes. It looks like a gutter might need to be added to the left (and possibly right) of the slider bar so that there's a little breathing room, and for consistency with the spacing controls (#44858). For example, in Storybook, the thumb lines up/overlaps the reset button slightly in the right-most position:
For the gutter between the UnitControl and the slider, there's a bit of discussion on @aaronrobertshaw's PR #44858 (comment) where @jasmussen mentioned 16px.
Similar to that PR, I noticed that in the 100% position, the tooltip results in a scrollbar being introduced in browsers that have visible scrollbars switched on — not sure if that's also something to be addressed via gutters 🤔 (I suspect it's because the thumb hangs over the edge of the space for the component?):
Last one: (not sure if this is something to be looked into separately), the click and drag behaviour on the UnitControl currently allows negative values to be used, so if you're scrolling down below 0, negative values are displayed and the slider pops into the middle position. Would it be worth adding a min value to the <FontSizePicker>?
Other than that, this looks very close to me!
| min={ isValueUnitRelative ? 0.75 : 12 } | ||
| max={ isValueUnitRelative ? 6.25 : 100 } | ||
| step={ isValueUnitRelative ? 0.05 : 1 } |
There was a problem hiding this comment.
These values feel nice to play with in the UI! 👍
|
I just rebased this onto trunk to get the latest fluid type commits and tested in TT3. I noticed that, for some custom fields, I can't change the unit:
Maybe it's something to do with the style value defined in theme.json. Here's the h2, which is a clamp value. And h4: a CSS var. And h1, which is a simple I can change the units for h1 but not h4 and h2. |
|
Thanks for the reviews! 🙇
Good flag! I added a 8px margin to the left and right of the slider. When combined with the existing 8px flex gap that makes for a 16px space between the unit control and slider. (We can't set the flex gap to 16px because then the unit control won't align with other single column controls in Global Styles.)
A
@ramonjd I can't reproduce what you see there when I test in Twenty Twenty-three. Want to double check? What am I missing? 😅 |
Did you rebase onto trunk? I just nuked my environment, then checked out this branch and rebased and can still reproduce.
Running WordPress 6.1-RC1-54506 I'm very willing to accept that it's just me by the way 😄 |
noisysocks
force-pushed
the
update/font-size-picker-slider
branch
from
001e0de to
ab52205
Compare
|
Thanks @ramonjd, I did indeed forget to rebase That bug should be fixed now. Turns out I was wrong about #44598 (comment). If we stop doing the naive check for Thinking about this, probably what makes more sense is to have the caller of |
|
Keen to get @ciampo to double check the code here but in particular my assumption that it's an "enhancement" and not a "breaking change" to change how the component looks when |
I can confirm that it's fixed, thanks! 2022-10-14.14.39.05.mp4
And have the incoming units populate the drop down? That makes sense! Agree it's a PR for "later Rob" 😄 I'm not sure if this a UX problem or not. If I want a font size of > 100 anything, the slider becomes a little redundant in that I can adjust around my selected value, only between 0-100. And if I touch the slider my custom font size is reset to 100. 2022-10-14.14.49.44.mp4My brain expected the slider to adjust the value in the UI control. I get that it might not be practical to have the slider adjust the range according to the custom value however, I'm not sure what's right here. |
|
Hm not sure what we could do about that. Dynamically set the max of the slider? It could get confusing then that different positions on the slider correspond to different values depending on what you previously entered. For what it's worth this is how the UnitControl + RangeSlider pattern works in other parts of the block editor that uses it. (Margins and padding, specifically.) |
|
Catching up with my review queue, I'll try to give this PR a thorough look next week — but in the meantime:
|
Ah that's interesting thanks. Maybe we could decide on a universal behaviour here and perform a sidebar-wide revisit in another PR? |
I'm not sure about this, but about updating the So, starting with a default max of
|
|
I do like that. But how about we implement that logic in |
noisysocks
force-pushed
the
update/font-size-picker-slider
branch
from
6495837 to
df15dc5
Compare
|
I rebased this, removed |
I've added this to the tracking issue and will look to work the dynamic max/step into the |
There was a problem hiding this comment.
LGTM.
This is working as described. I tested using different units and for elements, blocks in Global styles, as well as at the block-level.
Does this need a CHANGELOG entry?
Another possible follow up might be to see if adding touch-action: none; to the custom input numeric field improves interactivity on touch devices:
Without
With
This was something I'd added to the new SliderNumberControl etc. Didn't see any issues in initial testing but had flagged it for a call out when it comes time for reviews. |
Looks like there's an issue for this: #38865
Thanks, added! |
| const splitValuesCases: [ | ||
| number | string, | ||
| string | undefined, | ||
| string | undefined | ||
| ][] = [ | ||
| // Test integers and non-integers. | ||
| [ 1, '1', undefined ], | ||
| [ 1.25, '1.25', undefined ], | ||
| [ '123', '123', undefined ], | ||
| [ '1.5', '1.5', undefined ], | ||
| [ '0.75', '0.75', undefined ], | ||
| // Valid simple CSS values. | ||
| [ '20px', '20', 'px' ], | ||
| [ '0.8em', '0.8', 'em' ], | ||
| [ '2rem', '2', 'rem' ], | ||
| [ '1.4vw', '1.4', 'vw' ], | ||
| [ '0.4vh', '0.4', 'vh' ], | ||
| // Invalid negative values, | ||
| [ '-5px', undefined, undefined ], | ||
| // Complex CSS values that shouldn't parse. | ||
| [ 'abs(-15px)', undefined, undefined ], | ||
| [ 'calc(10px + 1)', undefined, undefined ], | ||
| [ 'clamp(2.5rem, 4vw, 3rem)', undefined, undefined ], | ||
| [ 'max(4.5em, 3vh)', undefined, undefined ], | ||
| [ 'min(10px, 1rem)', undefined, undefined ], | ||
| [ 'minmax(30px, auto)', undefined, undefined ], | ||
| [ 'var(--wp--font-size)', undefined, undefined ], | ||
| ]; | ||
|
|
||
| describe( 'splitValueAndUnitFromSize', () => { | ||
| test.each( splitValuesCases )( | ||
| 'given %p as argument, returns value = %p and unit = %p', | ||
| ( cssValue, expectedValue, expectedUnit ) => { | ||
| const [ value, unit ] = splitValueAndUnitFromSize( cssValue ); | ||
| expect( value ).toBe( expectedValue ); | ||
| expect( unit ).toBe( expectedUnit ); | ||
| } | ||
| ); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
Would it be useful to add these tests to the UnitControl utils that FontSizePicker is now reusing?
There was a problem hiding this comment.
Good plan. I'll spin up a PR.
What?
Updates the design and behaviour of
FontSizePickerwhenwithSlideris set to match the design.emandremvalues using the slider.Why?
Part of an effort to make the Typography panel match the design. See #34345 (comment).
How?
Testing Instructions
Screenshots or screencast
Global styles:
Storybook:
Storybook with Reset button:
This isn't used anywhere in Gutenberg (we could probably deprecate
withReset) but I ensured it looks reasonable as this is the default.Video:
Kapture.2022-09-30.at.15.46.05.mp4