gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

FontSizePicker: tidy up internal logic

Open ciampo opened this issue 1 year ago • 2 comments

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 trunk and on this PR
  • Test FontSizePicker in Storybook, especially around the props that influence what UI is presented to the user:
    • fontSizes (>= 5 font sizes => CustomSelectControl, < 5 font sizes => ToggleGroupControl)
    • disableCustomFontSizes should 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:

  1. Load the default FontSizePicker Storybook example
  2. Show the custom value UI by clicking on the toggle in the top-right corner
  3. Change the disableCustomFontSizes prop to true
  4. On trunk, no UI is presented to the user. On this PR, either ToggleGroupControl or CustomSelectControl UIs are shows based on how many font sizes the user can pick from.

ciampo avatar Jul 15 '24 08:07 ciampo

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.

github-actions[bot] avatar Jul 15 '24 08:07 github-actions[bot]

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
 	);
 

ciampo avatar Jul 16 '24 11:07 ciampo