From 418d8b77f1580677493d37077bdaa1b23e6879ea Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 21 Jun 2024 18:25:08 +0200 Subject: [PATCH] CustomSelectControl V2: fix passing external value/defaultValue (#62733) * Legavy v2 adapter: set value and defaultValue explicitly * Update v1 and legacy v2 tests to align assertions on onChange calls * Update unit tests * CHANGELOG * Move external controlled update tests outside of describe each * Re-use option objects when it makes sense instead of hardcoded strings --- Co-authored-by: ciampo Co-authored-by: tyxla --- packages/components/CHANGELOG.md | 3 +- .../legacy-component/index.tsx | 6 + .../legacy-component/test/index.tsx | 124 +++++++++++++----- .../src/custom-select-control/test/index.js | 99 ++++++++++---- 4 files changed, 168 insertions(+), 64 deletions(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 9a595827ffb0c8..57599320404e4b 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -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) diff --git a/packages/components/src/custom-select-control-v2/legacy-component/index.tsx b/packages/components/src/custom-select-control-v2/legacy-component/index.tsx index 089d55c9a0a06c..41be4f58d9147f 100644 --- a/packages/components/src/custom-select-control-v2/legacy-component/index.tsx +++ b/packages/components/src/custom-select-control-v2/legacy-component/index.tsx @@ -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( diff --git a/packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx b/packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx index 06af287a142367..f0b699617cd03c 100644 --- a/packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx +++ b/packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx @@ -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 ] ); const onChange: typeof onChangeProp = ( changeObject ) => { onChangeProp?.( changeObject ); @@ -83,12 +83,87 @@ const ControlledCustomSelectControl = ( { ); }; +it( 'Should apply external controlled updates', async () => { + const mockOnChange = jest.fn(); + const { rerender } = render( + + ); + + const currentSelectedItem = screen.getByRole( 'combobox', { + expanded: false, + } ); + + expect( currentSelectedItem ).toHaveTextContent( + legacyProps.options[ 0 ].name + ); + + expect( mockOnChange ).not.toHaveBeenCalled(); + + rerender( + + ); + + 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( ); + + 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( + + ); + + 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( ); @@ -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 ), } ) ); } ); @@ -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', @@ -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( { @@ -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 () => { @@ -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' ); diff --git a/packages/components/src/custom-select-control/test/index.js b/packages/components/src/custom-select-control/test/index.js index 2eb855c15b51f9..4a19449ac80730 100644 --- a/packages/components/src/custom-select-control/test/index.js +++ b/packages/components/src/custom-select-control/test/index.js @@ -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 ); @@ -81,12 +81,72 @@ const ControlledCustomSelectControl = ( { ); }; +it( 'Should apply external controlled updates', async () => { + const mockOnChange = jest.fn(); + const { rerender } = render( + + ); + + const currentSelectedItem = screen.getByRole( 'button', { + expanded: false, + } ); + + expect( currentSelectedItem ).toHaveTextContent( props.options[ 0 ].name ); + + rerender( + + ); + + 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( ); + + expect( + screen.getByRole( 'button', { + expanded: false, + } ) + ).toHaveTextContent( props.options[ 0 ].name ); + + 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( + + ); + + 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(); @@ -285,13 +345,7 @@ describe.each( [ const user = userEvent.setup(); const mockOnChange = jest.fn(); - render( - - ); + render( ); await user.click( screen.getByRole( 'button', { @@ -299,32 +353,21 @@ describe.each( [ } ) ); - // 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 ), } ) ); } ); @@ -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', @@ -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 () => {