FontSizePicker: tidy up internal logic
What?
Extract some of the changes from #63040 to tidy up FontSizePicker's internal logic
Why?
The component's internal logic for which UI to show is based on several boolean flags spread across the file, which makes it difficult to read/understand and bug-prone.
How?
This reactor extracts the logic to a few utility functions and condenses the UI state into one internal state variable.
Testing Instructions
- Make sure that the unit tests (including the ones added in this PR) also pass both on
trunkand on this PR - Test
FontSizePickerin Storybook, especially around the props that influence what UI is presented to the user:-
fontSizes(>= 5 font sizes =>CustomSelectControl, < 5 font sizes =>ToggleGroupControl) -
disableCustomFontSizesshould disable the custom UI and hide the toggle - experiment with changing those props while different UIs are presented
-
- Smoke test in the editor
[!NOTE] The new PR introduces a slight behavioral change to the component, which is IMO an improvement. Here's how to reproduce:
- Load the default
FontSizePickerStorybook example- Show the custom value UI by clicking on the toggle in the top-right corner
- Change the
disableCustomFontSizesprop totrue- On
trunk, no UI is presented to the user. On this PR, eitherToggleGroupControlorCustomSelectControlUIs are shows based on how many font sizes the user can pick from.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.
If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.
That's an interesting way to rethink the component's logic.
This will fail the "doesn't hide custom input when the selected font size is a predef" tests. However, I'm not sure this behavior is necessarily wrong? If the user never requested or interacted with the custom inputs, maybe it is more natural to fall back to the predef pickers if the value changed programatically. What do you think?
It's not necessarily wrong, but there may be value in showing the user the current value via the custom picker, rather than a blank togglegroup/select control. Imagine a user setting a custom font size, and later re-accessing those settings — wouldn't be weird not to see that custom font size reflected in the UI? It could give the wrong impression that the setting was not recorded.
Another unintended side-effect of the proposed change (which was apparently not covered by unit tests 😱 I added one! ) is that the user is not able to switch to predef inputs when a custom value (ie. non-predef) is entered in the custom UI:
https://github.com/user-attachments/assets/14ca326d-437b-42a8-89eb-0ec0a3c0ef39
I went ahead and made a tweak to the code you suggested (this diff is meant to be applied on top of the diff previously suggested by you), which should be closer to the original behavior (causing all existing unit tests to pass).
diff --git a/packages/components/src/font-size-picker/index.tsx b/packages/components/src/font-size-picker/index.tsx
index 8f5fba2be0..7085487adb 100644
--- a/packages/components/src/font-size-picker/index.tsx
+++ b/packages/components/src/font-size-picker/index.tsx
@@ -107,7 +107,9 @@ const UnforwardedFontSizePicker = (
);
const isCustomValue = !! value && ! selectedFontSize;
- const [ userRequestedCustom, setUserRequestedCustom ] = useState( false );
+ // Initially request a custom picker if the value is not from the predef list.
+ const [ userRequestedCustom, setUserRequestedCustom ] =
+ useState( isCustomValue );
const units = useCustomUnits( {
availableUnits: unitsProp,
@@ -120,7 +122,7 @@ const UnforwardedFontSizePicker = (
const currentPickerType = getPickerType(
// If showing the custom value picker, switch back to predef only
// if `disableCustomFontSizes` is set to `true`.
- ! disableCustomFontSizes && ( userRequestedCustom || isCustomValue ),
+ ! disableCustomFontSizes && userRequestedCustom,
fontSizes
);