FontSizePicker: tidy up internal logic #63553

ciampo · 2024-07-15T08:16:12Z

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:

Load the default FontSizePicker Storybook example
Show the custom value UI by clicking on the toggle in the top-right corner
Change the disableCustomFontSizes prop to true
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.

github-actions · 2024-07-15T08:29:24Z

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 <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ciampo · 2024-07-15T08:34:40Z

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

+			expect(
+				screen.queryByLabelText( 'Custom' )
+			).not.toBeInTheDocument();


I didn't write a more specific assertion on which UI is presented to the user since I initially wrote this unit test so that it would pass on trunk, where no input UI is actually presented to the user in this case (a bug IMO — see a more extended explanation in the PR description).

Since the input UI presented to the user changes depending on the length of the fontSizes prop, if we think that we should test that part too, I will duplicate this test and add it to two different sections of the file

ciampo · 2024-07-15T12:08:33Z

packages/components/CHANGELOG.md

@@ -6,17 +6,18 @@

 -   `Button`: Never apply `aria-disabled` to anchor ([#63376](https://github.com/WordPress/gutenberg/pull/63376)).

+### Enhancements


Moved the Enhancements section before the Internal section

mirka

I couldn't help but think the useEffect is avoidable 🤔 How I model this in my brain, there is only one bit of state ("is the user requesting a custom input"), and the currentPickerType can be derived without a useEffect/useState.

diff --git a/packages/components/src/font-size-picker/index.tsx b/packages/components/src/font-size-picker/index.tsx
index a547a26de5..8f5fba2be0 100644
--- a/packages/components/src/font-size-picker/index.tsx
+++ b/packages/components/src/font-size-picker/index.tsx
@@ -8,7 +8,7 @@ import type { ForwardedRef } from 'react';
  */
 import { __ } from '@wordpress/i18n';
 import { settings } from '@wordpress/icons';
-import { useState, useEffect, forwardRef } from '@wordpress/element';
+import { useState, forwardRef } from '@wordpress/element';
 
 /**
  * Internal dependencies
@@ -107,27 +107,7 @@ const UnforwardedFontSizePicker = (
 	);
 	const isCustomValue = !! value && ! selectedFontSize;
 
-	const [ currentPickerType, setCurrentPickerType ] = useState(
-		getPickerType( ! disableCustomFontSizes && isCustomValue, fontSizes )
-	);
-
-	useEffect( () => {
-		setCurrentPickerType(
-			getPickerType(
-				// If showing the custom value picker, switch back to predef only
-				// if `disableCustomFontSizes` is set to `true`.
-				currentPickerType === 'custom'
-					? ! disableCustomFontSizes
-					: ! disableCustomFontSizes && isCustomValue,
-				fontSizes
-			)
-		);
-	}, [
-		currentPickerType,
-		disableCustomFontSizes,
-		isCustomValue,
-		fontSizes,
-	] );
+	const [ userRequestedCustom, setUserRequestedCustom ] = useState( false );
 
 	const units = useCustomUnits( {
 		availableUnits: unitsProp,
@@ -137,6 +117,13 @@ const UnforwardedFontSizePicker = (
 		return null;
 	}
 
+	const currentPickerType = getPickerType(
+		// If showing the custom value picker, switch back to predef only
+		// if `disableCustomFontSizes` is set to `true`.
+		! disableCustomFontSizes && ( userRequestedCustom || isCustomValue ),
+		fontSizes
+	);
+
 	const headerHint = getHeaderHint(
 		currentPickerType,
 		selectedFontSize,
@@ -180,14 +167,9 @@ const UnforwardedFontSizePicker = (
 									: __( 'Set custom size' )
 							}
 							icon={ settings }
-							onClick={ () => {
-								setCurrentPickerType(
-									getPickerType(
-										currentPickerType !== 'custom',
-										fontSizes
-									)
-								);
-							} }
+							onClick={ () =>
+								setUserRequestedCustom( ! userRequestedCustom )
+							}
 							isPressed={ currentPickerType === 'custom' }
 							size="small"
 						/>
@@ -215,9 +197,7 @@ const UnforwardedFontSizePicker = (
 								);
 							}
 						} }
-						onSelectCustom={ () =>
-							setCurrentPickerType( 'custom' )
-						}
+						onSelectCustom={ () => setUserRequestedCustom( true ) }
 					/>
 				) }
 				{ currentPickerType === 'togglegroup' && (
@@ -251,6 +231,8 @@ const UnforwardedFontSizePicker = (
 								hideLabelFromVision
 								value={ value }
 								onChange={ ( newValue ) => {
+									setUserRequestedCustom( true );
+
 									if ( newValue === undefined ) {
 										onChange?.( undefined );
 									} else {
@@ -281,6 +263,8 @@ const UnforwardedFontSizePicker = (
 										initialPosition={ fallbackFontSize }
 										withInputField={ false }
 										onChange={ ( newValue ) => {
+											setUserRequestedCustom( true );
+
 											if ( newValue === undefined ) {
 												onChange?.( undefined );
 											} else if ( hasUnits ) {

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?

mirka · 2024-07-15T15:45:37Z

packages/components/src/font-size-picker/types.ts

@@ -29,24 +31,25 @@ export type FontSizePickerProps = {
 	/**
 	 * Available units for custom font size selection.
 	 *
-	 * @default `[ 'px', 'em', 'rem' ]`
+	 * @default [ 'px', 'em', 'rem' ]


I noticed this was out of date, fix at #63577.

tyxla · 2024-07-16T09:42:17Z

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

+								setCurrentPickerType(
+									getPickerType(
+										currentPickerType !== 'custom',
+										fontSizes


If we moved the getPickerType and getHeaderHint inside the component function, we wouldn't need to pass fontSizes all over the place. We could also potentially reuse some of the conditions (like whether the current picker type is custom for example), or even remove some of the utilities (like shouldUseSelectOverToggle()). Maybe that could reduce some of the complexity?

I had initially extracted those function to a separate utility to better isolate the key pieces of logic and make the render function easier to scan.

But after applying Lena's advice, I noticed that some extra code could be removed — and so I put them back in the render function:

41deb9f

c53036a

tyxla · 2024-07-16T09:43:28Z

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

+				T_SHIRT_NAMES[ fontSizes.indexOf( selectedFontSize ) ]
+			);
+		}
+		return '';


I know this is pre-existing, but I wasn't sure why in this case we don't display the common unit (like we do below) and if this is intentional.

I'm not sure if this was intentional or not — I would probably need to look in the git history and see if this aspect was discussed actively in previous PRs. Probably out of scope for this refactor, though — for now, I've decided to keep the '' fallback around to preserve the same behavior as on trunk.

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

tyxla · 2024-07-16T09:57:25Z

packages/components/src/font-size-picker/types.ts

 	 *
 	 * @default false
 	 */
 	withSlider?: boolean;
 	/**
 	 * If `true`, a reset button will be displayed alongside the input field
 	 * when a custom font size is active. Has no effect when
-	 * `disableCustomFontSizes` or `withSlider` is `true`.


Good catch, seems like this was wrong.

ciampo · 2024-07-16T11:26:39Z

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:

Kapture.2024-07-16.at.12.14.23.mp4

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 · 2024-07-16T13:55:28Z

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

-		if ( commonUnit ) {
-			return `(${ commonUnit })`;
+	const headerHint = useMemo( () => {
+		switch ( currentPickerType ) {


Swapped the previous if-based logic to a switch-based logic looking at the value of currentPickerType, which should be much simpler to follow

mirka

Looks good and tests well, thanks for the refinements! The logic is now easier to follow, we have better test coverage, and we have better behavior for the edge cases 🚀

…actor to switch statement.

* Add unit tests * Refactor code and extract logic * Sync JS docs * CHANGELOG * Further simplify logic * Inline `getPickerType` utility * Inline `getHeaderHint` and `shouldUseSelectOverToggle` utilities, refactor to switch statement. * toBeInTheDocument() => toBeVisible() * Move units back at the start of the render function for a smaller diff --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>

ciampo self-assigned this Jul 15, 2024

ciampo mentioned this pull request Jul 15, 2024

FontSizePicker: add pickerMode #63040

Closed

ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jul 15, 2024

ciampo requested a review from a team July 15, 2024 08:29

ciampo marked this pull request as ready for review July 15, 2024 08:29

ciampo requested a review from ajitbohra as a code owner July 15, 2024 08:29

ciampo commented Jul 15, 2024

View reviewed changes

mirka reviewed Jul 15, 2024

View reviewed changes

tyxla reviewed Jul 16, 2024

View reviewed changes

ciampo force-pushed the refactor/tidy-up-font-size-picker branch from 36d04b2 to 7d0b69d Compare July 16, 2024 11:26

ciampo requested review from mirka and tyxla July 16, 2024 11:37

ciampo force-pushed the refactor/tidy-up-font-size-picker branch from 7d67b56 to 1340d11 Compare July 16, 2024 12:38

ciampo commented Jul 16, 2024

View reviewed changes

mirka approved these changes Jul 17, 2024

View reviewed changes

ciampo added 9 commits July 18, 2024 09:32

Add unit tests

d9f7d87

Refactor code and extract logic

eb5ae09

Sync JS docs

feb36c9

CHANGELOG

0e8f1c4

Further simplify logic

b0e91a0

Inline getPickerType utility

dd5f9c3

Inline getHeaderHint and shouldUseSelectOverToggle utilities, ref…

c707d1d

…actor to switch statement.

toBeInTheDocument() => toBeVisible()

a9b1953

Move units back at the start of the render function for a smaller diff

30e5c64

ciampo force-pushed the refactor/tidy-up-font-size-picker branch from 1340d11 to 30e5c64 Compare July 18, 2024 07:36

ciampo enabled auto-merge (squash) July 18, 2024 07:37

ciampo merged commit 777ebc1 into trunk Jul 18, 2024
61 checks passed

ciampo deleted the refactor/tidy-up-font-size-picker branch July 18, 2024 08:31

github-actions bot added this to the Gutenberg 18.9 milestone Jul 18, 2024

jhallbachner mentioned this pull request Oct 15, 2024

Font size abbreviations in FontSizePicker are misleading when the theme declares multiple "small" sizes. #44245

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FontSizePicker: tidy up internal logic #63553

FontSizePicker: tidy up internal logic #63553

ciampo commented Jul 15, 2024 •

edited

Loading

github-actions bot commented Jul 15, 2024 •

edited

Loading

ciampo Jul 15, 2024 •

edited

Loading

ciampo Jul 15, 2024

mirka left a comment

mirka Jul 15, 2024

tyxla Jul 16, 2024

ciampo Jul 16, 2024

tyxla Jul 16, 2024

ciampo Jul 16, 2024 •

edited

Loading

tyxla Jul 16, 2024

ciampo commented Jul 16, 2024 •

edited

Loading

ciampo Jul 16, 2024

mirka left a comment

		@@ -6,17 +6,18 @@

		- `Button`: Never apply `aria-disabled` to anchor ([#63376](https://github.com/WordPress/gutenberg/pull/63376)).

		### Enhancements

FontSizePicker: tidy up internal logic #63553

FontSizePicker: tidy up internal logic #63553

Conversation

ciampo commented Jul 15, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jul 15, 2024 • edited Loading

ciampo Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

ciampo Jul 15, 2024

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

mirka Jul 15, 2024

Choose a reason for hiding this comment

tyxla Jul 16, 2024

Choose a reason for hiding this comment

ciampo Jul 16, 2024

Choose a reason for hiding this comment

tyxla Jul 16, 2024

Choose a reason for hiding this comment

ciampo Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

tyxla Jul 16, 2024

Choose a reason for hiding this comment

ciampo commented Jul 16, 2024 • edited Loading

ciampo Jul 16, 2024

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

ciampo commented Jul 15, 2024 •

edited

Loading

github-actions bot commented Jul 15, 2024 •

edited

Loading

ciampo Jul 15, 2024 •

edited

Loading

ciampo Jul 16, 2024 •

edited

Loading

ciampo commented Jul 16, 2024 •

edited

Loading