-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FontSizePicker: Use components instead of helper functions #44891
Conversation
Simplify how FontSizePicker is implemented by using nested components instead of helper functions. Components are easier to reason about and much easier to type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
tofont-size-picker-toggle-group.tsx
select.tsx
tofont-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 totrue
, whenwithSlider
is also set totrue
- The logic around
fallbackFontSize
,withReset
andwithSlider
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 thedisableCustomFontSizes
totrue
iii. The UI disappears and the user is stuck
Thanks for the feedback @ciampo!
I think #44598 should accidentally fix this!
The whole component is very confusing 😂. I'd be keen to deprecate |
Feedback addressed or responded to! Thanks again. |
Thank you! I will try to have another look at this before EOW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!)
I rebased this and addressed all the inline feedback. I'll improve the |
…y-font-size-picker
I stacked this PR onto #45298. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Nice catch @ciampo, I've fixed that and added regression tests. |
I tricked myself, I think :) |
} ); | ||
} | ||
|
||
export function getSelectedOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
🐎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, I can probably use selectedFontSize
or something similar. Never mind me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah selectedFontSize
should work. Or you can fontSizes.find()
. Happy to help you with the rebase if it's too gnarly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Great, great work here. Thank you so much!
What?
Simplify how
FontSizePicker
is implemented by using nested components for theCustomSelectControl
andToggleGroupControl
logic instead of helper functions.Why?
While adding types to
FontSizePicker
in #44449 I ran into difficulty typinggetFontSizeOptions
as its return type depends on the runtime value ofuseSelectControl
. 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
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 theCustomSelectControl
(>5 font sizes) andToggleGroupControl
(≤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?
FontSizePickerSelect
component: this contains theCustomSelectControl
and logic for when there are > 5 font sizes.FontSizePickerToggleGroup
component: this contains theToggleGroupControl
and logic for when there are ≤5 font sizes.utils.ts
.Testing Instructions
Either:
FontSizePicker
in Storybook, or;The font size picker should work exactly as it does in
trunk
and tests should pass.