Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToggleGroupControl: Don't set value on focus after a reset #66151

Merged
merged 4 commits into from
Oct 16, 2024
Merged

ToggleGroupControl: Don't set value on focus after a reset #66151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a85cd8
Add test
mirka Oct 15, 2024
65e2a9b
ToggleGroupControl: Don't set value on focus after a reset
mirka Oct 15, 2024
1c8b689
Add changelog
mirka Oct 16, 2024
c550dec
Rename to `selectedValueIsEmpty`
mirka Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- `Tabs`: fix skipping indication animation glitch ([#65878](https://github.com/WordPress/gutenberg/pull/65878)).
- `ToggleGroupControl`: Don't autoselect option on first group focus ([#65892](https://github.com/WordPress/gutenberg/pull/65892)).
- `Button`: fix `box-shadow` transition for secondary variation ([#66045](https://github.com/WordPress/gutenberg/pull/66045)).
- `ToggleGroupControl`: Don't set value on focus after a reset ([#66151](https://github.com/WordPress/gutenberg/pull/66151)).

### Deprecations

Expand Down
26 changes: 26 additions & 0 deletions packages/components/src/toggle-group-control/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,32 @@ describe.each( [
expect( radio ).not.toBeChecked();
} );

if ( mode === 'controlled' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move it next to the other controlled-only tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion, but I think I prefer it here because it's closely related to the test right above it.

it( 'should not set a value on focus, after the value is reset', async () => {
render(
<Component label="Test Toggle Group Control" value="jack">
{ options }
</Component>
);

expect( screen.getByRole( 'radio', { name: 'J' } ) ).toBeChecked();

await click( screen.getByRole( 'button', { name: 'Reset' } ) );

expect(
screen.getByRole( 'radio', { name: 'J' } )
).not.toBeChecked();

await press.ShiftTab();
expect(
screen.getByRole( 'radio', { name: 'R' } )
).not.toBeChecked();
expect(
screen.getByRole( 'radio', { name: 'J' } )
).not.toBeChecked();
} );
}

it( 'should render tooltip where `showTooltip` === `true`', async () => {
render(
<Component label="Test Toggle Group Control">
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,14 @@ function ToggleGroupControlOptionBase(
<Ariakit.Radio
disabled={ disabled }
onFocusVisible={ () => {
const selectedValueIsEmpty =
toggleGroupControlContext.value === null ||
toggleGroupControlContext.value === '';
Comment on lines +145 to +146
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to how we add a layer of value conversion in controlled mode, the Ariakit store will hold a null value on first render with no value provided, but then an empty string after a consumer resets the value to undefined.


// Conditions ensure that the first visible focus to a radio group
// without a selected option will not automatically select the option.
if (
toggleGroupControlContext.value !== null ||
! selectedValueIsEmpty ||
toggleGroupControlContext.activeItemIsNotFirstItem?.()
) {
toggleGroupControlContext.setValue( value );
Expand Down
9 changes: 8 additions & 1 deletion packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { useStoreState } from '@ariakit/react';
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { forwardRef, useMemo } from '@wordpress/element';
import { forwardRef, useEffect, useMemo } from '@wordpress/element';
import { isRTL } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -73,6 +73,13 @@ function UnforwardedToggleGroupControlAsRadioGroup(
const selectedValue = useStoreState( radio, 'value' );
const setValue = radio.setValue;

// Ensures that the active id is also reset after the value is "reset" by the consumer.
useEffect( () => {
if ( selectedValue === '' ) {
radio.setActiveId( undefined );
}
}, [ radio, selectedValue ] );
Comment on lines +76 to +81
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can feel the tech debt catching up to us 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Food for thought: I wonder if this can be improved with a centralized controlled component layer that all controlled components would use. Probably a too idealistic idea, since some components use Ariakit and others do not, and this layer would be quite complicated to cover all cases.

Copy link
Contributor

@ciampo ciampo Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the most practical fix here is to change the APIs to follow the standard convention for controlled / uncontrolled component. Since that would be a breaking change, we could just release a V2 of the component that would give us also a chance to:

  • implement / test the new design spec alongside the legacy one
  • review the current API strategy around "deselectability" and potentially multiple selection (should they be separate components? Or different subcomponents?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case specifically, I think there might be room to argue that resetting the active id on empty-ish (or invalid) values should be handled by Ariakit out of the box. I'll try and follow up.


const groupContextValue = useMemo(
(): ToggleGroupControlContextProps => ( {
activeItemIsNotFirstItem: () =>
Expand Down
Loading