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

CustomSelectControl V2: fix setting initial value and reacting to external controlled updates #62733

Merged
merged 6 commits into from
Jun 21, 2024
Merged

CustomSelectControl V2: fix setting initial value and reacting to external controlled updates #62733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8673249
Legavy v2 adapter: set value and defaultValue explicitly
ciampo Jun 21, 2024
517fdfd
Update v1 and legacy v2 tests to align assertions on onChange calls
ciampo Jun 21, 2024
ba35e3b
Update unit tests
ciampo Jun 21, 2024
fe36b16
CHANGELOG
ciampo Jun 21, 2024
a073327
Move external controlled update tests outside of describe each
ciampo Jun 21, 2024
c5b6a4c
Re-use option objects when it makes sense instead of hardcoded strings
ciampo Jun 21, 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
3 changes: 2 additions & 1 deletion packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
### Internal

- `CustomSelectControl`: align unit tests for v1 and legacy v2 versions. ([#62706](https://github.com/WordPress/gutenberg/pull/62706))
- `CustomSelectControl`: fix handling of extra option attributes in the `onChange` callbacks and when forwarding them to the option DOM elements. ([#62255](https://github.com/WordPress/gutenberg/pull/62255))
- `CustomSelectControlV2`: fix handling of extra option attributes in the `onChange` callbacks and when forwarding them to the option DOM elements. ([#62255](https://github.com/WordPress/gutenberg/pull/62255))
- `CustomSelectControlV2`: fix setting initial value and reacting to external controlled updates. ([#62733](https://github.com/WordPress/gutenberg/pull/62733))

## 28.1.0 (2024-06-15)

Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/custom-select-control-v2/legacy-component/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
};
onChange( changeObject );
},
value: value?.name,
// Setting the first option as a default value when no value is provided
// is already done natively by the underlying Ariakit component,
// but doing this explicitly avoids the `onChange` callback from firing
// on initial render, thus making this implementation closer to the v1.
defaultValue: options[ 0 ]?.name,
} );

const children = options.map(
Expand Down
124 changes: 88 additions & 36 deletions packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const ControlledCustomSelectControl = ( {
onChange: onChangeProp,
...restProps
}: React.ComponentProps< typeof UncontrolledCustomSelectControl > ) => {
const [ value, setValue ] = useState( options[ 0 ] );
const [ value, setValue ] = useState( restProps.value ?? options[ 0 ] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change allows the Controlled version of the component to use the value prop as the initial value (the same change was applied to the v1 component)


const onChange: typeof onChangeProp = ( changeObject ) => {
onChangeProp?.( changeObject );
Expand All @@ -83,12 +83,87 @@ const ControlledCustomSelectControl = ( {
);
};

it( 'Should apply external controlled updates', async () => {
const mockOnChange = jest.fn();
const { rerender } = render(
<UncontrolledCustomSelectControl
{ ...legacyProps }
value={ legacyProps.options[ 0 ] }
onChange={ mockOnChange }
/>
);

const currentSelectedItem = screen.getByRole( 'combobox', {
expanded: false,
} );

expect( currentSelectedItem ).toHaveTextContent(
legacyProps.options[ 0 ].name
);

expect( mockOnChange ).not.toHaveBeenCalled();

rerender(
<UncontrolledCustomSelectControl
{ ...legacyProps }
value={ legacyProps.options[ 1 ] }
/>
);

expect( currentSelectedItem ).toHaveTextContent(
legacyProps.options[ 1 ].name
);

// Necessary to wait for onChange to potentially fire
await sleep();

expect( mockOnChange ).not.toHaveBeenCalled();
} );

describe.each( [
[ 'Uncontrolled', UncontrolledCustomSelectControl ],
[ 'Controlled', ControlledCustomSelectControl ],
] )( 'CustomSelectControl (%s)', ( ...modeAndComponent ) => {
const [ , Component ] = modeAndComponent;

it( 'Should select the first option when no explicit initial value is passed without firing onChange', async () => {
const mockOnChange = jest.fn();
render( <Component { ...legacyProps } onChange={ mockOnChange } /> );

expect(
screen.getByRole( 'combobox', {
expanded: false,
} )
).toHaveTextContent( legacyProps.options[ 0 ].name );

// Necessary to wait for onChange to potentially fire
await sleep();

expect( mockOnChange ).not.toHaveBeenCalled();
} );

it( 'Should pick the initially selected option if the value prop is passed without firing onChange', async () => {
const mockOnChange = jest.fn();
render(
<Component
{ ...legacyProps }
onChange={ mockOnChange }
value={ legacyProps.options[ 3 ] }
/>
);

expect(
screen.getByRole( 'combobox', {
expanded: false,
} )
).toHaveTextContent( legacyProps.options[ 3 ].name );

// Necessary to wait for onChange to potentially fire
await sleep();

expect( mockOnChange ).not.toHaveBeenCalled();
} );

it( 'Should replace the initial selection when a new item is selected', async () => {
render( <Component { ...legacyProps } /> );

Expand Down Expand Up @@ -292,33 +367,21 @@ describe.each( [
} )
);

// NOTE: legacy CustomSelectControl doesn't fire onChange
// at this point in time.
expect( mockOnChange ).toHaveBeenNthCalledWith(
1,
expect.objectContaining( {
inputValue: '',
isOpen: false,
selectedItem: { key: 'flower1', name: 'violets' },
type: '',
} )
);

await click(
screen.getByRole( 'option', {
name: 'aquamarine',
} )
);

expect( mockOnChange ).toHaveBeenNthCalledWith(
2,
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
expect( mockOnChange ).toHaveBeenLastCalledWith(
expect.objectContaining( {
inputValue: '',
isOpen: false,
selectedItem: expect.objectContaining( {
name: 'aquamarine',
} ),
type: '',
type: expect.any( String ),
Copy link
Member

Choose a reason for hiding this comment

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

type will literally always be an empty string, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will, but since we don't really document it in the v1, I thought that tests should be generic about what value to expect — as far as the attribute exists and it's a string. This also allows for the v1 and legacy v2 unit tests to be aligned on this particular assertion

} )
);
} );
Expand All @@ -336,23 +399,11 @@ describe.each( [
} )
).toHaveFocus();

// NOTE: legacy CustomSelectControl doesn't fire onChange
// at this point in time.
expect( mockOnChange ).toHaveBeenNthCalledWith(
1,
expect.objectContaining( {
selectedItem: expect.objectContaining( {
key: 'flower1',
name: 'violets',
} ),
} )
);

await type( 'p' );
await press.Enter();

expect( mockOnChange ).toHaveBeenNthCalledWith(
2,
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
expect( mockOnChange ).toHaveBeenLastCalledWith(
expect.objectContaining( {
selectedItem: expect.objectContaining( {
key: 'flower3',
Expand Down Expand Up @@ -387,10 +438,7 @@ describe.each( [

await click( optionWithCustomAttributes );

// NOTE: legacy CustomSelectControl doesn't fire onChange
// on first render, and so at this point in time `onChangeMock`
// would be called only once.
expect( onChangeMock ).toHaveBeenCalledTimes( 2 );
expect( onChangeMock ).toHaveBeenCalledTimes( 1 );
expect( onChangeMock ).toHaveBeenCalledWith(
expect.objectContaining( {
selectedItem: expect.objectContaining( {
Expand Down Expand Up @@ -454,7 +502,9 @@ describe.each( [
await press.ArrowDown();
await press.Enter();

expect( currentSelectedItem ).toHaveTextContent( 'crimson clover' );
expect( currentSelectedItem ).toHaveTextContent(
legacyProps.options[ 1 ].name
);
} );

it( 'Should be able to type characters to select matching options', async () => {
Expand Down Expand Up @@ -488,7 +538,9 @@ describe.each( [
await sleep();
await press.Tab();
expect( currentSelectedItem ).toHaveFocus();
expect( currentSelectedItem ).toHaveTextContent( 'violets' );
expect( currentSelectedItem ).toHaveTextContent(
legacyProps.options[ 0 ].name
);

// Ideally we would test a multi-character typeahead, but anything more than a single character is flaky
await type( 'a' );
Expand Down
99 changes: 72 additions & 27 deletions packages/components/src/custom-select-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const ControlledCustomSelectControl = ( {
onChange: onChangeProp,
...restProps
} ) => {
const [ value, setValue ] = useState( options[ 0 ] );
const [ value, setValue ] = useState( restProps.value ?? options[ 0 ] );

const onChange = ( changeObject ) => {
onChangeProp?.( changeObject );
Expand All @@ -81,12 +81,72 @@ const ControlledCustomSelectControl = ( {
);
};

it( 'Should apply external controlled updates', async () => {
const mockOnChange = jest.fn();
const { rerender } = render(
<UncontrolledCustomSelectControl
{ ...props }
value={ props.options[ 0 ] }
onChange={ mockOnChange }
/>
);

const currentSelectedItem = screen.getByRole( 'button', {
expanded: false,
} );

expect( currentSelectedItem ).toHaveTextContent( props.options[ 0 ].name );

rerender(
<UncontrolledCustomSelectControl
{ ...props }
value={ props.options[ 1 ] }
/>
);

expect( currentSelectedItem ).toHaveTextContent( props.options[ 1 ].name );

expect( mockOnChange ).not.toHaveBeenCalled();
} );

describe.each( [
[ 'uncontrolled', UncontrolledCustomSelectControl ],
[ 'controlled', ControlledCustomSelectControl ],
[ 'Uncontrolled', UncontrolledCustomSelectControl ],
[ 'Controlled', ControlledCustomSelectControl ],
] )( 'CustomSelectControl %s', ( ...modeAndComponent ) => {
const [ , Component ] = modeAndComponent;

it( 'Should select the first option when no explicit initial value is passed without firing onChange', () => {
const mockOnChange = jest.fn();
render( <Component { ...props } onChange={ mockOnChange } /> );

expect(
screen.getByRole( 'button', {
expanded: false,
} )
).toHaveTextContent( props.options[ 0 ].name );

expect( mockOnChange ).not.toHaveBeenCalled();
Copy link
Member

Choose a reason for hiding this comment

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

No delay / sleep necessary?

This applies to the rest of the places where we're asserting whether onChange callback was called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my tests, it was not necessary for the v1 version, while it was unfortunately necessary for the ariakit-based version

} );

it( 'Should pick the initially selected option if the value prop is passed without firing onChange', async () => {
const mockOnChange = jest.fn();
render(
<Component
{ ...props }
onChange={ mockOnChange }
value={ props.options[ 3 ] }
/>
);

expect(
screen.getByRole( 'button', {
expanded: false,
} )
).toHaveTextContent( props.options[ 3 ].name );

expect( mockOnChange ).not.toHaveBeenCalled();
} );

it( 'Should replace the initial selection when a new item is selected', async () => {
const user = userEvent.setup();

Expand Down Expand Up @@ -285,46 +345,29 @@ describe.each( [
const user = userEvent.setup();
const mockOnChange = jest.fn();

render(
<Component
{ ...props }
value={ props.options[ 0 ] }
onChange={ mockOnChange }
/>
);
render( <Component { ...props } onChange={ mockOnChange } /> );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Align the test with the legacy v2 adapter (which doesn't set the value prop)


await user.click(
screen.getByRole( 'button', {
expanded: false,
} )
);

// DIFFERENCE WITH V2: NOT CALLED
// expect( mockOnChange ).toHaveBeenNthCalledWith(
// 1,
// expect.objectContaining( {
// inputValue: '',
// isOpen: false,
// selectedItem: { key: 'flower1', name: 'violets' },
// type: '',
// } )
// );

await user.click(
screen.getByRole( 'option', {
name: 'aquamarine',
} )
);

expect( mockOnChange ).toHaveBeenNthCalledWith(
1,
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
expect( mockOnChange ).toHaveBeenLastCalledWith(
expect.objectContaining( {
inputValue: '',
isOpen: false,
selectedItem: expect.objectContaining( {
name: 'aquamarine',
} ),
type: '__item_click__',
type: expect.any( String ),
Copy link
Member

Choose a reason for hiding this comment

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

To make sure I understand this change: it's for compatibility so the both versions of the tests would have the same expectation against the different implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! See reply in #62733 (comment)

} )
);
} );
Expand All @@ -345,8 +388,8 @@ describe.each( [
await user.keyboard( 'p' );
await user.keyboard( '{enter}' );

expect( mockOnChange ).toHaveBeenNthCalledWith(
1,
expect( mockOnChange ).toHaveBeenCalledTimes( 1 );
expect( mockOnChange ).toHaveBeenLastCalledWith(
expect.objectContaining( {
selectedItem: expect.objectContaining( {
key: 'flower3',
Expand Down Expand Up @@ -446,7 +489,9 @@ describe.each( [
await user.keyboard( '{arrowdown}' );
await user.keyboard( '{enter}' );

expect( currentSelectedItem ).toHaveTextContent( 'crimson clover' );
expect( currentSelectedItem ).toHaveTextContent(
props.options[ 1 ].name
);
} );

it( 'Should be able to type characters to select matching options', async () => {
Expand Down
Loading