CustomSelectControl: Update Value from Fresh State #67733

subodhr258 · 2024-12-09T10:35:39Z

What?

This PR fixes the issue where the font size select box display was not updating immediately when the font size was reset to the default state (fontsize was being set to undefined). The showSelectedHint logic in the CustomSelectControl was interfering with the immediate update of the displayed value.

Why?

When the font size was reset, the showSelectedHint function attempted to render additional UI elements conditionally. In the case when the value was set to undefined, the dropdown display was not updated properly.

How?

This bug is now fixed by prioritizing the value from the props instead of using the state or passing it as an argument. The renderSelectedValueHint function was adjusted to handle the value from props:

Replaced the reliance on currentValue with a direct check on value?.name to determine the selected option and its hint.
The renderSelectedValueHint function now properly reflects changes when the font size is reset, and the dropdown updates immediately.

Testing Instructions

Add font size presets.
Go to block editor.
Add a paragraph to the document.
Open the Typography panel.
Change the font size.
Click “reset” to clear the font size.

Testing Instructions for Keyboard

Screenshots or screencast

Screencast of the fixed issue:

Screen.Recording.2024-12-09.at.3.51.28.PM.mov

github-actions · 2024-12-09T10:35:53Z

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: subodhr258 <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: sarthaknagoshe2002 <[email protected]>
Co-authored-by: Rishit30G <[email protected]>
Co-authored-by: inc2734 <[email protected]>

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

github-actions · 2024-12-09T10:36:28Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @subodhr258! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

mirka

Thank you for contributing! Please add a changelog, too.

I also noticed a related bug, but let's deal with it separately (#67759).

mirka · 2024-12-09T20:01:36Z

packages/components/src/custom-select-control/index.tsx

@@ -154,11 +154,11 @@ function CustomSelectControl< T extends CustomSelectOption >(
 	const renderSelectedValueHint = () => {
 		const selectedOptionHint = options
 			?.map( applyOptionDeprecations )
-			?.find( ( { name } ) => currentValue === name )?.hint;
+			?.find( ( { name } ) => value?.name === name )?.hint;


This is not "prioritizing the value from the props", it's replacing the value from store with the value from props. The failing unit test tells us that it's a breaking change.

The problem has to do with stale state, due to this component (CustomSelectControl) not re-rendering after a selection change when in uncontrolled mode. Since renderSelectedValueHint does get re-executed when _CustomSelect itself re-renders, we can change the code to get fresh state on each call:

Suggested change

?.find( ( { name } ) => value?.name === name )?.hint;

?.find( ( { name } ) => store.getState().value === name )?.hint;

mirka · 2024-12-09T20:02:10Z

packages/components/src/custom-select-control/index.tsx


 		return (
 			<Styled.SelectedExperimentalHintWrapper>
-				{ currentValue }
+				{ value?.name }


Suggested change

{ value?.name }

{ store.getState().value }

sarthaknagoshe2002 · 2024-12-09T21:16:57Z

I think the argument to the function getDescribedBy on line 208 should also be same as the value in function renderSelectedValueHint to ensure consistency in the selected font and the font name in the screen reader text

mirka · 2024-12-09T23:32:53Z

I think the argument to the function getDescribedBy on line 208 should also be same as the value in function renderSelectedValueHint to ensure consistency in the selected font and the font name in the screen reader text

Right, though in the interest of getting this PR merged as soon as possible, I extracted that issue out to #67759 because it likely requires a separate solution and testing.

…ale state

subodhr258 · 2024-12-10T06:09:32Z

Hi @mirka!
I have applied the suggested change to get the fresh state on each call, and also updated the changelog. Self-tested the change to see if the issue was fixed and the CustomSelectControl was indeed updated immediately on reset.
Thanks!

sarthaknagoshe2002 · 2024-12-10T09:55:23Z

I’ve tested this, and the PR LGTM!
Thanks @subodhr258 for the fix! 🚀

mirka

Looks good, thank you!

mirka · 2024-12-10T17:43:38Z

packages/components/CHANGELOG.md

@@ -44,6 +43,7 @@
 ### Bug Fixes

 -   `ResizableBox`: Make drag handles focusable ([#67305](https://github.com/WordPress/gutenberg/pull/67305)).
+-   `CustomSelectControl`: Update correctly when `showSelectedHint` is enabled ([#67733](https://github.com/WordPress/gutenberg/pull/67733)).


Tweaked the text a bit so it describes the change to any consumer of this component, not a user of the Gutenberg app.

…ss#67733

CustomSelectControl: Prioritized props value over store

aa91dff

subodhr258 requested a review from ajitbohra as a code owner December 9, 2024 10:35

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Dec 9, 2024

CustomSelectControl: Refactored Codebase to fix failing testcases

8fc395f

Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system labels Dec 9, 2024

Mamaduka requested a review from a team December 9, 2024 11:20

mirka reviewed Dec 9, 2024

View reviewed changes

subodhr258 added 2 commits December 10, 2024 11:04

docs: Add changelog entry for CustomSelectControl font size reset fix

4401676

CustomSelectControl: Fetch the fresh state on each call instead of st…

734808f

…ale state

subodhr258 changed the title ~~CustomSelectControl: Prioritized props value over store~~ CustomSelectControl: Update Value from Fresh State Dec 10, 2024

Tweak changelog text

89b198d

mirka approved these changes Dec 10, 2024

View reviewed changes

Merge branch 'trunk' into 67721-fix-fontsize-reset

8bf3a0a

mirka enabled auto-merge (squash) December 10, 2024 17:45

mirka added [Package] Components /packages/components and removed [Feature] UI Components Impacts or related to the UI component system labels Dec 10, 2024

mirka merged commit 9387206 into WordPress:trunk Dec 10, 2024
65 checks passed

github-actions bot added this to the Gutenberg 19.9 milestone Dec 10, 2024

yogeshbhutkar mentioned this pull request Dec 11, 2024

CustomSelectControl: Refactor to use Ariakit store state for current value #67815

Merged

yogeshbhutkar added a commit to yogeshbhutkar/gutenberg that referenced this pull request Dec 11, 2024

CustomSelectControl: Refactor and revert code changes made in WordPre…

39ac45a

…ss#67733

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControl: Update Value from Fresh State #67733

CustomSelectControl: Update Value from Fresh State #67733

subodhr258 commented Dec 9, 2024

github-actions bot commented Dec 9, 2024 •

edited

Loading

github-actions bot commented Dec 9, 2024

mirka left a comment

mirka Dec 9, 2024

mirka Dec 9, 2024

sarthaknagoshe2002 commented Dec 9, 2024

mirka commented Dec 9, 2024

subodhr258 commented Dec 10, 2024

sarthaknagoshe2002 commented Dec 10, 2024 •

edited

Loading

mirka left a comment

mirka Dec 10, 2024

	?.find( ( { name } ) => value?.name === name )?.hint;
	?.find( ( { name } ) => store.getState().value === name )?.hint;

CustomSelectControl: Update Value from Fresh State #67733

CustomSelectControl: Update Value from Fresh State #67733

Conversation

subodhr258 commented Dec 9, 2024

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Dec 9, 2024 • edited Loading

github-actions bot commented Dec 9, 2024

mirka left a comment

Choose a reason for hiding this comment

mirka Dec 9, 2024

Choose a reason for hiding this comment

mirka Dec 9, 2024

Choose a reason for hiding this comment

sarthaknagoshe2002 commented Dec 9, 2024

mirka commented Dec 9, 2024

subodhr258 commented Dec 10, 2024

sarthaknagoshe2002 commented Dec 10, 2024 • edited Loading

mirka left a comment

Choose a reason for hiding this comment

mirka Dec 10, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2024 •

edited

Loading

sarthaknagoshe2002 commented Dec 10, 2024 •

edited

Loading