FontSizePicker: Use components instead of helper functions by noisysocks · Pull Request #44891 · WordPress/gutenberg

noisysocks · 2022-10-12T06:05:18Z

What?

Simplify how FontSizePicker is implemented by using nested components for the CustomSelectControl and ToggleGroupControl logic instead of helper functions.

Why?

While adding types to FontSizePicker in #44449 I ran into difficulty typing getFontSizeOptions as its return type depends on the runtime value of useSelectControl. See #44449 (comment) for some relevant discussion there.

The end result is lots of yucky as casts scattered throughout the component:

gutenberg/packages/components/src/font-size-picker/index.tsx

Line 218 in 3983fee

options={ options as FontSizeSelectOption[] }

In addition to being difficult to type, it's just plain confusing. @ramonjd learned this the other day when trying to make a simple change to FontSizePicker in #44791. He only needed to change one line but the design of this component tricked him into thinking it was more complicated than it was. (Sorry to pick on you @ramonjd.)

To do away with all of this, I've settled on embracing that FontSizePicker works differently depending on if there are more than 5 font sizes or not. I've split out the CustomSelectControl (>5 font sizes) and ToggleGroupControl (≤5 font sizes) into their own components. Now each component can do whatever it needs to do without having to worry about the other. We can then remove the difficult-to-type helper functions altogether.

How?

New FontSizePickerSelect component: this contains the CustomSelectControl and logic for when there are > 5 font sizes.
New FontSizePickerToggleGroup component: this contains the ToggleGroupControl and logic for when there are ≤5 font sizes.
Removed no-longer-necessary helpers from utils.ts.

Testing Instructions

Either:

Look at FontSizePicker in Storybook, or;
Go to Site Editor → Global Styles → Typography → Text → Size.

The font size picker should work exactly as it does in trunk and tests should pass.

Simplify how FontSizePicker is implemented by using nested components instead of helper functions. Components are easier to reason about and much easier to type.

ciampo

Thank you for working on this, @noisysocks !

This has been a much deeper review than anticipated 😅

Apart from the inline comments, could we also rename:

toggle-group.tsx to font-size-picker-toggle-group.tsx
select.tsx to font-size-picker-select.tsx

The following observations are also present on trunk, but I thought I'd mention them anyway:

I noticed that the user can still enter a custom value even if disableCustomFontSizes is set to true, when withSlider is also set to true
The logic around fallbackFontSize, withReset and withSlider is very confusing and should be reviewed.
The component can get into a weird state if:
i. Click on the toggle to show the custom font size controls
ii. Change the disableCustomFontSizes to true
iii. The UI disappears and the user is stuck

noisysocks · 2022-10-13T04:39:52Z

Thanks for the feedback @ciampo!

I noticed that the user can still enter a custom value even if disableCustomFontSizes is set to true, when withSlider is also set to true

I think #44598 should accidentally fix this!

The logic around fallbackFontSize, withReset and withSlider is very confusing and should be reviewed.

The whole component is very confusing 😂. I'd be keen to deprecate withReset. fallbackFontSize is named poorly (suggestion: defaultSliderValue) but probably needs to be there.

noisysocks · 2022-10-13T05:53:19Z

Feedback addressed or responded to! Thanks again.

ciampo · 2022-10-13T09:22:48Z

Thank you! I will try to have another look at this before EOW

ciampo

There's a couple of comments still in need of addressing, but in the meantime I gave the component a look on Storybook and it looks fine!

I still think that we could add more tests to FontSizePicker, given its complexity and the number of edge cases (an example of the need for more coverage is that my manual code review flagged at least 3 instances in which the original logic was not refactored correctly). It would be a good investment , since from what I understand this component will be changed/improved further in the upcoming weeks.

(ps: for the same reasons, more manual testing is welcome too!)

…-size-picker

noisysocks · 2022-10-25T05:33:51Z

I rebased this and addressed all the inline feedback. I'll improve the FontSizePicker tests tomorrow in a seperate PR so that we can merge that one first.

…hensive

…y-font-size-picker

noisysocks · 2022-11-01T05:26:08Z

I stacked this PR onto #45298.

noisysocks · 2022-11-01T06:31:29Z

Cool, the unit tests added in #45298 pass! That should reassure us that this refactor doesn't break FontSizePicker. How are you feeling @ciampo?

ciampo

How are you feeling @ciampo?

Feeling pretty good about this change and the newly added test — they definitely give us more confidence when making changes moving forward!

Although I still found some discrepancies between this PR and trunk, which I outlined in one of the inline conversations. I believe that once that's solved, we'll be ready to merge!

noisysocks · 2022-11-02T02:32:03Z

Nice catch @ciampo, I've fixed that and added regression tests.

ramonjd · 2022-11-02T05:20:36Z

He only needed to change one line but the design of this component tricked him into thinking it was more complicated than it was.

I tricked myself, I think :)

ramonjd · 2022-11-02T05:21:38Z

-	} );
-}
-
-export function getSelectedOption(


Heads up, it's possible that I'll need this again for #44967

Maybe not. Either way, it's going to be a gnarly rebase if this goes in first.

🐎

Oh wait, I can probably use selectedFontSize or something similar. Never mind me.

Yeah selectedFontSize should work. Or you can fontSizes.find(). Happy to help you with the rebase if it's too gnarly.

ciampo

LGTM 🚀

Great, great work here. Thank you so much!

FontSizePicker: Use components instead of helper functions

dc4b183

Simplify how FontSizePicker is implemented by using nested components instead of helper functions. Components are easier to reason about and much easier to type.

noisysocks added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Oct 12, 2022

noisysocks requested a review from ajitbohra as a code owner October 12, 2022 06:05

noisysocks requested a review from ciampo October 12, 2022 06:05

Remove duplicate dependency header

bbf17a3

noisysocks commented Oct 12, 2022

View reviewed changes

Comment thread packages/components/src/font-size-picker/test/utils.ts

ciampo requested a review from mirka October 12, 2022 09:46

ciampo assigned noisysocks Oct 12, 2022

ciampo requested a review from chad1008 October 12, 2022 09:46

ciampo reviewed Oct 12, 2022

View reviewed changes

noisysocks added 7 commits October 13, 2022 15:48

Reuse types from FontSizePickerProps where possible

450d63b

Rename 'number' to 'parsedValue'

c50fa22

Use constants for defaultOption and customOption

f882797

Don't show 'Custom' option in select when custom sizes are disabled

b108a6e

Swap label and aria-label

4cdf21c

Use slug in select if name is an empty string

c76c8e6

Add test that checks all the t-shirt labels

c5e4fae

noisysocks mentioned this pull request Oct 14, 2022

FontSizePicker: Update hint text to match the design #44966

Merged

ciampo reviewed Oct 14, 2022

View reviewed changes

Comment thread packages/components/src/font-size-picker/test/index.tsx Outdated

noisysocks added 5 commits October 25, 2022 15:36

Merge remote-tracking branch 'origin/trunk' into update/simplify-font…

6b26dc2

…-size-picker

Make filenames match component names

97e572a

Make DEFAULT_OPTION logic more explicit

1de7d4e

Restore headerHint logic to how it was

10bf17e

Use expect().toHaveContent

8f87273

Rewrite FontSizePicker unit tests to use userEvent and be more compre…

299d680

…hensive

noisysocks mentioned this pull request Oct 26, 2022

FontSizePicker: Rewrite unit tests to use userEvent and be more comprehensive #45298

Merged

noisysocks added 13 commits October 28, 2022 15:50

Use header label's aria-label

b0314d2

Put back the Gigantosaurus 🦖

c2b4da6

Update changelog

d6c4d65

Merge branch 'trunk' into update/font-size-picker-unit-tests

9b9fc00

Remove editorialising comment

a939507

Use getByLabelText( 'Custom Size' ) to select slider

22c9ab3

Remove redundant test

8a0fc89

Add tests for non-px font sizes

1673ba3

Assert length of options/radios

ab89dde

Assert number of calls to onChange

4032f0f

Merge branch 'update/font-size-picker-unit-tests' into update/simplif…

41006fc

…y-font-size-picker

Can un-comment this failing assertion now

591288b

Update CHANGELOG.md

3af78ec

noisysocks changed the base branch from trunk to update/font-size-picker-unit-tests November 1, 2022 05:25

Base automatically changed from update/font-size-picker-unit-tests to trunk November 1, 2022 06:20

Merge branch 'trunk' into update/simplify-font-size-picker

bdef23a

ciampo reviewed Nov 1, 2022

View reviewed changes

Comment thread packages/components/src/font-size-picker/index.tsx

Fix regression when value=""

639db98

ramonjd reviewed Nov 2, 2022

View reviewed changes

ciampo approved these changes Nov 2, 2022

View reviewed changes

noisysocks merged commit 3081460 into trunk Nov 2, 2022

noisysocks deleted the update/simplify-font-size-picker branch November 2, 2022 20:55

github-actions Bot added this to the Gutenberg 14.6 milestone Nov 2, 2022

Conversation

noisysocks commented Oct 12, 2022

What?

Why?

How?

Testing Instructions

Uh oh!

Uh oh!

ciampo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noisysocks commented Oct 13, 2022

Uh oh!

noisysocks commented Oct 13, 2022

Uh oh!

ciampo commented Oct 13, 2022

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

noisysocks commented Oct 25, 2022

Uh oh!

noisysocks commented Nov 1, 2022

Uh oh!

noisysocks commented Nov 1, 2022

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

noisysocks commented Nov 2, 2022

Uh oh!

ramonjd commented Nov 2, 2022

Uh oh!

ramonjd Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

ramonjd Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

noisysocks Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ciampo left a comment •

edited

Loading