FontSizePicker: Fix FontSizePicker Storybook control type and improve documentation #68936

himanshupathak95 · 2025-01-29T08:13:29Z

What?

This PR fixes an issue with the FontSizePicker component in Storybook where changing the units prop was causing an error. It also clarifies the documentation around the component's unitless mode.

Why?

When trying to change the units prop in Storybook, an error was occurring: allowedUnitValues.includes is not a function. This happened because clicking the Set object button in Storybook initialized the value as an empty object instead of an array. Additionally, the documentation didn't clearly explain when the units prop would actually have an effect.

How?

Changes the Storybook control type for the units prop from the default to inline-check with predefined options
Improves the README documentation to clearly explain the relationship between string/number font-size values and the unitless mode

Testing Instructions

Run Storybook with npm run storybook:dev
Navigate to the FontSizePicker component
Verify you can select different unit options using the checkboxes without any errors
Verify that the component's documentation explains that the units property only works when font sizes are specified as strings with units

Screencast

Screen.Recording.2025-02-26.at.12.43.14.mov

himanshupathak95 · 2025-01-29T09:54:33Z

Hi, @t-hamano I have currently applied your fix and it seems to solve the issue but I am finding better possibilities.
This might not be the final solution but I think this is a discussion starter.
I will be updating the README and the storybook in the following commits.

Meanwhile, if we find any better solutions, we can explore them here.

t-hamano · 2025-02-01T05:09:19Z

packages/components/src/font-size-picker/README.md

@@ -92,6 +92,8 @@ Size of the control.

 Available units for custom font size selection.

+**Important Note**: For the units property to work, font sizes must be specified as strings with units (e.g., `'12px'` instead of `12`). When font sizes are provided as numbers, the component operates in "unitless mode" where the units property has no effect.


Suggested change

**Important Note**: For the units property to work, font sizes must be specified as strings with units (e.g., `'12px'` instead of `12`). When font sizes are provided as numbers, the component operates in "unitless mode" where the units property has no effect.

**Note**: For the `units` property to work, the current font size value must be specified as strings with units (e.g., `'12px'` instead of `12`). When the font size is provided as a number, the component operates in "unitless mode" where the `units` property has no effect.

My feeling is that this should be emphasized in the value prop section, what do you think?

Yes @t-hamano you're right, It makes more sense to emphasize the value prop selection.

t-hamano · 2025-02-01T05:17:11Z

packages/components/src/unit-control/utils.ts

@@ -394,7 +394,7 @@ export function filterUnitsWithSettings(
 	// Although the `isArray` check shouldn't be necessary (given the signature of
 	// this typed function), it's better to stay on the side of caution, since
 	// this function may be called from un-typed environments.
-	return Array.isArray( availableUnits )
+	return Array.isArray( availableUnits ) && Array.isArray( allowedUnitValues )


I'm not too confident about this change. Because it silently squashes errors and doesn't match the expected type (string[]).

That said, it does fix the error when trying to change the units prop in Storybook. This error occurs because when you press the "Set object" button, the value is initialized as an empty object instead of an empty array.

cc @WordPress/gutenberg-components

I haven't tried, but would it make sense to change the Storybook control type to something like inline-check or multi-select?

Thanks for the feedback! I've tried both controls and it feels like the inline-check control is the better fit.

inline-check;

multi-select:

…iltering

github-actions · 2025-02-26T07:17:32Z

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: himanshupathak95 <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: SainathPoojary <[email protected]>
Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: slaFFik <[email protected]>

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

t-hamano · 2025-02-26T13:06:19Z

packages/components/src/font-size-picker/README.md

@@ -92,6 +92,8 @@ Size of the control.

 Available units for custom font size selection.

+**Note**: For the `units` property to work, the current font size value must be specified as strings with units (e.g., `'12px'` instead of `12`). When the font size is provided as a number, the component operates in "unitless mode" where the `units` property has no effect.


What do you think about moving this to the value prop section? Because unitless mode is based on the value prop value, not the units prop value.

t-hamano · 2025-02-26T13:10:08Z

packages/components/CHANGELOG.md

@@ -2,6 +2,8 @@

 ## Unreleased

+-   `FontSizePicker`: Fix Storybook units control type to use `inline-check` and improve documentation clarifying unitless mode in `README.md` ([#68936](https://github.com/WordPress/gutenberg/pull/68936)).


Suggested change

- `FontSizePicker`: Fix Storybook units control type to use `inline-check` and improve documentation clarifying unitless mode in `README.md` ([#68936](https://github.com/WordPress/gutenberg/pull/68936)).

### Documentation

- `FontSizePicker`: Fix Storybook units control type to use `inline-check` and improve documentation clarifying unitless mode in `README.md` ([#68936](https://github.com/WordPress/gutenberg/pull/68936)).

The checklog should be included in a subheading. "Documentation" would be good.

shail-mehta added [Type] Developer Documentation Documentation for developers [Package] Components /packages/components Storybook Storybook and its stories for components labels Jan 30, 2025

t-hamano reviewed Feb 1, 2025

View reviewed changes

himanshupathak95 added 4 commits February 26, 2025 12:26

Components: Improve FontSizePicker units documentation and fix unit f…

7851f6a

…iltering

Fix failing unit tests

79e66a7

Docs: Update FontSizePicker README.md to mention the unitless mode

a8b6ec5

FontSizePicker: Change the storybook control type to inline-check

32086da

himanshupathak95 force-pushed the fix/68553-font-size-picker-unit-filtering branch from 746016f to 32086da Compare February 26, 2025 06:56

FontSizePicker: Add entry in CHANGELOG.md

2a6ccb4

himanshupathak95 marked this pull request as ready for review February 26, 2025 07:17

himanshupathak95 requested a review from ajitbohra as a code owner February 26, 2025 07:17

himanshupathak95 requested review from mirka and t-hamano February 26, 2025 07:17

himanshupathak95 changed the title ~~Components: Improve FontSizePicker units documentation and fix unit filtering~~ FontSizePicker: Fix FontSizePicker Storybook control type and improve documentation Feb 26, 2025

t-hamano reviewed Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FontSizePicker: Fix FontSizePicker Storybook control type and improve documentation #68936

FontSizePicker: Fix FontSizePicker Storybook control type and improve documentation #68936

himanshupathak95 commented Jan 29, 2025 •

edited

Loading

himanshupathak95 commented Jan 29, 2025

t-hamano Feb 1, 2025

himanshupathak95 Feb 26, 2025

t-hamano Feb 1, 2025

mirka Feb 3, 2025

t-hamano Feb 4, 2025

github-actions bot commented Feb 26, 2025

t-hamano Feb 26, 2025

t-hamano Feb 26, 2025

		@@ -92,6 +92,8 @@ Size of the control.

		Available units for custom font size selection.

		Important Note: For the units property to work, font sizes must be specified as strings with units (e.g., `'12px'` instead of `12`). When font sizes are provided as numbers, the component operates in "unitless mode" where the units property has no effect.

	Important Note: For the units property to work, font sizes must be specified as strings with units (e.g., `'12px'` instead of `12`). When font sizes are provided as numbers, the component operates in "unitless mode" where the units property has no effect.
	Note: For the `units` property to work, the current font size value must be specified as strings with units (e.g., `'12px'` instead of `12`). When the font size is provided as a number, the component operates in "unitless mode" where the `units` property has no effect.

		@@ -2,6 +2,8 @@

		## Unreleased

		- `FontSizePicker`: Fix Storybook units control type to use `inline-check` and improve documentation clarifying unitless mode in `README.md` ([#68936](https://github.com/WordPress/gutenberg/pull/68936)).

FontSizePicker: Fix FontSizePicker Storybook control type and improve documentation #68936

Are you sure you want to change the base?

FontSizePicker: Fix FontSizePicker Storybook control type and improve documentation #68936

Conversation

himanshupathak95 commented Jan 29, 2025 • edited Loading

What?

Why?

How?

Testing Instructions

Screencast

himanshupathak95 commented Jan 29, 2025

t-hamano Feb 1, 2025

Choose a reason for hiding this comment

himanshupathak95 Feb 26, 2025

Choose a reason for hiding this comment

t-hamano Feb 1, 2025

Choose a reason for hiding this comment

mirka Feb 3, 2025

Choose a reason for hiding this comment

t-hamano Feb 4, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2025

t-hamano Feb 26, 2025

Choose a reason for hiding this comment

t-hamano Feb 26, 2025

Choose a reason for hiding this comment

himanshupathak95 commented Jan 29, 2025 •

edited

Loading