From 4c549a54c19d08e78915f2e854e9a12f65eb8aac Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Wed, 17 Apr 2024 18:42:33 -0600 Subject: [PATCH 01/18] Pass the an object with the expected interface to the `selectedItem` property of the `changeObject` that's passed to the `onChange` callback for the legacy adapter --- .../font-appearance-control/index.js | 10 +++- .../legacy-component/index.tsx | 20 +++++-- .../legacy-component/test/index.tsx | 57 ++++++++++++++++++- packages/components/src/index.ts | 4 +- 4 files changed, 81 insertions(+), 10 deletions(-) diff --git a/packages/block-editor/src/components/font-appearance-control/index.js b/packages/block-editor/src/components/font-appearance-control/index.js index 18e814ad23ddb..c3629e902ef90 100644 --- a/packages/block-editor/src/components/font-appearance-control/index.js +++ b/packages/block-editor/src/components/font-appearance-control/index.js @@ -151,6 +151,10 @@ export default function FontAppearanceControl( props ) { key: value, name, style: { fontStyle: undefined, fontWeight: value }, + className: 'yay-custom-class-name-yolo', + arbitrary: 'property', + foo: 'bar', + bar: 'foooo', } ); } ); return combinedOptions; @@ -212,9 +216,9 @@ export default function FontAppearanceControl( props ) { describedBy={ getDescribedBy() } options={ selectOptions } value={ currentSelection } - onChange={ ( { selectedItem } ) => - onChange( selectedItem.style ) - } + onChange={ ( { selectedItem } ) => { + onChange( selectedItem.style ); + } } /> ) ); 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 43f102e6ee049..588b6aaaba5f7 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 @@ -36,16 +36,15 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { await Promise.resolve(); const state = store.getState(); + const option = options.find( ( item ) => item.name === nextValue ); + const changeObject = { highlightedIndex: state.renderedItems.findIndex( ( item ) => item.value === nextValue ), inputValue: '', isOpen: state.open, - selectedItem: { - name: nextValue as string, - key: nextValue as string, - }, + selectedItem: option!, type: '', }; onChange( changeObject ); @@ -121,4 +120,15 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { ); } -export default CustomSelectControl; +export function ClassicCustomSelectControlV2Adapter( + props: LegacyCustomSelectProps +) { + return ( + + ); +} + +export default ClassicCustomSelectControlV2Adapter; 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 02680e76cdb81..1e8a19603ddaa 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 @@ -54,6 +54,7 @@ const ControlledCustomSelectControl = ( { onChange: onChangeProp, ...restProps }: React.ComponentProps< typeof UncontrolledCustomSelectControl > ) => { + const { value: overrideValue } = restProps; const [ value, setValue ] = useState( options[ 0 ] ); const onChange: typeof onChangeProp = ( changeObject ) => { @@ -67,7 +68,7 @@ const ControlledCustomSelectControl = ( { options={ options } onChange={ onChange } value={ options.find( - ( option: any ) => option.key === value.key + ( option: any ) => option.key === initialValue.key ) } /> ); @@ -306,6 +307,7 @@ describe.each( [ expect.objectContaining( { inputValue: '', isOpen: false, + selectedItem: expect.objectContaining( { name: 'aquamarine', } ), @@ -537,4 +539,57 @@ describe.each( [ expect( onBlurMock ).toHaveBeenCalledTimes( 1 ); } ); } ); + + // V1 styles items via a `style` or `className` metadata property in the option item object. Some consumers still expect it, e.g: + // + // - https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/font-appearance-control/index.js#L216 + // + // Returning these properties as part of the item object was not tested as part of the V1 test. Possibly this was an accidental API? + // or was it intentional? If intentional, we might need to implement something similar in V2, too? The alternative is to rely on the + // `key` attriute for the item and get the actual data from some dictionary in a store somewhere, which would require refactoring + // consumers that rely on the self-contained `style` and `className` attributes. + it( 'Should return style metadata as part of the selected option from onChange', async () => { + const mockOnChange = jest.fn(); + + render( ); + + await click( + screen.getByRole( 'combobox', { + expanded: false, + } ) + ); + + await click( + screen.getByRole( 'option', { + name: 'aquarela', + } ) + ); + + expect( mockOnChange ).toHaveBeenCalledWith( + expect.objectContaining( { + selectedItem: expect.objectContaining( { + className: customClassName, + style: customStyles, + } ), + } ) + ); + } ); + + it( 'Should display the initial value passed as the selected value', async () => { + const initialSelectedItem = legacyProps.options[ 5 ]; + + const testProps = { + ...legacyProps, + value: initialSelectedItem, + }; + + render( ); + + const currentSelectedItem = await screen.findByRole( 'combobox', { + expanded: false, + } ); + + // Verify the initial selected value + expect( currentSelectedItem ).toHaveTextContent( 'aquarela' ); + } ); } ); diff --git a/packages/components/src/index.ts b/packages/components/src/index.ts index f3643a1499a02..0dc58bcc65714 100644 --- a/packages/components/src/index.ts +++ b/packages/components/src/index.ts @@ -63,7 +63,9 @@ export { useCompositeState as __unstableUseCompositeState, } from './composite'; export { ConfirmDialog as __experimentalConfirmDialog } from './confirm-dialog'; -export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control'; +//export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control'; +export { ClassicCustomSelectControlV2Adapter as CustomSelectControl } from './custom-select-control-v2/legacy-component'; +export { default as CustomSelectControlV2 } from './custom-select-control-v2'; export { default as Dashicon } from './dashicon'; export { default as DateTimePicker, DatePicker, TimePicker } from './date-time'; export { default as __experimentalDimensionControl } from './dimension-control'; From 827343506ef5374ad868be1ae11d0191c8ebf8cb Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Mon, 3 Jun 2024 19:11:00 -0600 Subject: [PATCH 02/18] Remove unrelated test for this part --- .../legacy-component/test/index.tsx | 18 ------------------ 1 file changed, 18 deletions(-) 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 1e8a19603ddaa..4a4e59cd9b317 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 @@ -574,22 +574,4 @@ describe.each( [ } ) ); } ); - - it( 'Should display the initial value passed as the selected value', async () => { - const initialSelectedItem = legacyProps.options[ 5 ]; - - const testProps = { - ...legacyProps, - value: initialSelectedItem, - }; - - render( ); - - const currentSelectedItem = await screen.findByRole( 'combobox', { - expanded: false, - } ); - - // Verify the initial selected value - expect( currentSelectedItem ).toHaveTextContent( 'aquarela' ); - } ); } ); From 038a278f619f897a7073f91f05c4ff7300f44c87 Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Mon, 3 Jun 2024 19:18:56 -0600 Subject: [PATCH 03/18] Remove debug code --- .../src/components/font-appearance-control/index.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/block-editor/src/components/font-appearance-control/index.js b/packages/block-editor/src/components/font-appearance-control/index.js index c3629e902ef90..18e814ad23ddb 100644 --- a/packages/block-editor/src/components/font-appearance-control/index.js +++ b/packages/block-editor/src/components/font-appearance-control/index.js @@ -151,10 +151,6 @@ export default function FontAppearanceControl( props ) { key: value, name, style: { fontStyle: undefined, fontWeight: value }, - className: 'yay-custom-class-name-yolo', - arbitrary: 'property', - foo: 'bar', - bar: 'foooo', } ); } ); return combinedOptions; @@ -216,9 +212,9 @@ export default function FontAppearanceControl( props ) { describedBy={ getDescribedBy() } options={ selectOptions } value={ currentSelection } - onChange={ ( { selectedItem } ) => { - onChange( selectedItem.style ); - } } + onChange={ ( { selectedItem } ) => + onChange( selectedItem.style ) + } /> ) ); From 498acf3160289a3683249884e645973d1cfbf4d5 Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Mon, 3 Jun 2024 19:38:38 -0600 Subject: [PATCH 04/18] Add test to V1 to confirm it supports passing custom properties as part of an option --- .../src/custom-select-control/test/index.js | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/packages/components/src/custom-select-control/test/index.js b/packages/components/src/custom-select-control/test/index.js index 867571650ed4e..63172f1e8ad32 100644 --- a/packages/components/src/custom-select-control/test/index.js +++ b/packages/components/src/custom-select-control/test/index.js @@ -247,28 +247,56 @@ describe.each( [ ).toHaveTextContent( 'Hint' ); } ); - it( 'shows selected hint in list of options when added, regardless of __experimentalShowSelectedHint prop', async () => { + it( 'Should accept and pass arbitrary properties to the selectedItem object in the onChange callback', async () => { const user = userEvent.setup(); + const onChangeMock = jest.fn(); render( ); + const currentSelectedItem = screen.getByRole( 'button', { + expanded: false, + } ); + + await user.click( currentSelectedItem ); await user.click( - screen.getByRole( 'button', { name: 'Custom select' } ) + screen.getByRole( 'option', { name: 'Custom Option' } ) ); - expect( screen.getByRole( 'option', { name: /hint/i } ) ).toBeVisible(); + expect( onChangeMock ).toHaveBeenCalledTimes( 1 ); + expect( onChangeMock ).toHaveBeenCalledWith( + expect.objectContaining( { + selectedItem: expect.objectContaining( { + key: 'custom', + name: 'Custom Option', + className: 'custom-class-name', + customProp1: 'value1', + customProp2: 42, + style: { + backgroundColor: 'rgb(127, 255, 212)', + rotate: '13deg', + }, + } ), + } ) + ); } ); it( 'Should return object onChange', async () => { From 16f09ebab1cf168d9c16c4ff319fce5454059ff0 Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Tue, 4 Jun 2024 12:45:52 -0600 Subject: [PATCH 05/18] Update V2 legacy adapter test to test that the comppnent supports custom attributes as part of option and that they are not added to the DOM as attributes (same behavior as V1) --- .../legacy-component/index.tsx | 5 ++-- .../legacy-component/test/index.tsx | 28 +++++++++---------- 2 files changed, 17 insertions(+), 16 deletions(-) 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 588b6aaaba5f7..fa411f29bcc1f 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 @@ -52,7 +52,7 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { } ); const children = options.map( - ( { name, key, __experimentalHint, ...rest } ) => { + ( { name, key, __experimentalHint, style, className } ) => { const withHint = ( { name } @@ -67,7 +67,8 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { key={ key } value={ name } children={ __experimentalHint ? withHint : name } - { ...rest } + style={ style } + className={ className } /> ); } 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 4a4e59cd9b317..0cdbcf1df9702 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 @@ -54,7 +54,6 @@ const ControlledCustomSelectControl = ( { onChange: onChangeProp, ...restProps }: React.ComponentProps< typeof UncontrolledCustomSelectControl > ) => { - const { value: overrideValue } = restProps; const [ value, setValue ] = useState( options[ 0 ] ); const onChange: typeof onChangeProp = ( changeObject ) => { @@ -68,7 +67,7 @@ const ControlledCustomSelectControl = ( { options={ options } onChange={ onChange } value={ options.find( - ( option: any ) => option.key === initialValue.key + ( option: any ) => option.key === value.key ) } /> ); @@ -541,14 +540,11 @@ describe.each( [ } ); // V1 styles items via a `style` or `className` metadata property in the option item object. Some consumers still expect it, e.g: - // // - https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/font-appearance-control/index.js#L216 - // - // Returning these properties as part of the item object was not tested as part of the V1 test. Possibly this was an accidental API? - // or was it intentional? If intentional, we might need to implement something similar in V2, too? The alternative is to rely on the - // `key` attriute for the item and get the actual data from some dictionary in a store somewhere, which would require refactoring - // consumers that rely on the self-contained `style` and `className` attributes. - it( 'Should return style metadata as part of the selected option from onChange', async () => { + // Besides that, the `option` prop is documented as havin the type: + // - `Array<{ key: String, name: String, style: ?{}, className: ?String, ...rest }>` + // Notice the `...test` there. We should keep supporting the arbitrary props like this. + it( 'Should return style and custom metadata as part of the selected option from onChange', async () => { const mockOnChange = jest.fn(); render( ); @@ -559,11 +555,15 @@ describe.each( [ } ) ); - await click( - screen.getByRole( 'option', { - name: 'aquarela', - } ) - ); + const optionElement = screen.getByRole( 'option', { + name: 'aquarela', + } ); + + // Assert that the option element does not have the custom attributes + expect( optionElement ).not.toHaveAttribute( 'customPropFoo' ); + expect( optionElement ).not.toHaveAttribute( 'customPropBar' ); + + await click( optionElement ); expect( mockOnChange ).toHaveBeenCalledWith( expect.objectContaining( { From e9587e231d37112969333f74920c661606a6b11a Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Tue, 4 Jun 2024 12:47:56 -0600 Subject: [PATCH 06/18] Type `LegacyOption` to support arbitrary attributes --- packages/components/src/custom-select-control-v2/types.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/src/custom-select-control-v2/types.ts b/packages/components/src/custom-select-control-v2/types.ts index 5540a533c09d4..12b41ba54f4a2 100644 --- a/packages/components/src/custom-select-control-v2/types.ts +++ b/packages/components/src/custom-select-control-v2/types.ts @@ -79,6 +79,7 @@ type LegacyOption = { style?: React.CSSProperties; className?: string; __experimentalHint?: string; + [ key: string ]: any; }; /** From cdac6f885fef18024ed1bf3cea8af2ef19ea492c Mon Sep 17 00:00:00 2001 From: Marcelo Serpa Date: Tue, 4 Jun 2024 12:49:04 -0600 Subject: [PATCH 07/18] Remove re-export for the adapter for now --- packages/components/src/index.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/components/src/index.ts b/packages/components/src/index.ts index 0dc58bcc65714..1ec3ae567108d 100644 --- a/packages/components/src/index.ts +++ b/packages/components/src/index.ts @@ -63,8 +63,7 @@ export { useCompositeState as __unstableUseCompositeState, } from './composite'; export { ConfirmDialog as __experimentalConfirmDialog } from './confirm-dialog'; -//export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control'; -export { ClassicCustomSelectControlV2Adapter as CustomSelectControl } from './custom-select-control-v2/legacy-component'; +export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control'; export { default as CustomSelectControlV2 } from './custom-select-control-v2'; export { default as Dashicon } from './dashicon'; export { default as DateTimePicker, DatePicker, TimePicker } from './date-time'; From 463948fd4434f39c19eb2d8cfd267a41f9f1d615 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 10:29:08 +0200 Subject: [PATCH 08/18] Remove non-nullable operator in favour of an early return --- .../legacy-component/index.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 fa411f29bcc1f..df0fcd1a44a9b 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 @@ -27,7 +27,11 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { // Forward props + store from v2 implementation const store = Ariakit.useSelectStore( { async setValue( nextValue ) { - if ( ! onChange ) { + const nextOption = options.find( + ( item ) => item.name === nextValue + ); + + if ( ! onChange || ! nextOption ) { return; } @@ -36,15 +40,13 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { await Promise.resolve(); const state = store.getState(); - const option = options.find( ( item ) => item.name === nextValue ); - const changeObject = { highlightedIndex: state.renderedItems.findIndex( ( item ) => item.value === nextValue ), inputValue: '', isOpen: state.open, - selectedItem: option!, + selectedItem: nextOption, type: '', }; onChange( changeObject ); From 29d1e41d2b3c291e9cf3e6fd3617f490a4f372e3 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 10:31:43 +0200 Subject: [PATCH 09/18] Remove extra space --- .../src/custom-select-control-v2/legacy-component/test/index.tsx | 1 - 1 file changed, 1 deletion(-) 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 0cdbcf1df9702..8696da43aa3a9 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 @@ -306,7 +306,6 @@ describe.each( [ expect.objectContaining( { inputValue: '', isOpen: false, - selectedItem: expect.objectContaining( { name: 'aquamarine', } ), From e8e9a22a0309125bfc959b1e17324a6b57c425d5 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 10:43:19 +0200 Subject: [PATCH 10/18] Set default false value via prop destructuring --- .../legacy-component/index.tsx | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) 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 df0fcd1a44a9b..089d55c9a0a06 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 @@ -14,7 +14,7 @@ import * as Styled from '../styles'; function CustomSelectControl( props: LegacyCustomSelectProps ) { const { - __experimentalShowSelectedHint, + __experimentalShowSelectedHint = false, __next40pxDefaultSize = false, describedBy, options, @@ -123,15 +123,4 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { ); } -export function ClassicCustomSelectControlV2Adapter( - props: LegacyCustomSelectProps -) { - return ( - - ); -} - -export default ClassicCustomSelectControlV2Adapter; +export default CustomSelectControl; From 320a13b4a32d269e1542fe75dd9cfb5601fa7b3e Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 11:01:22 +0200 Subject: [PATCH 11/18] Use a valid HTML attribute in CustomSelectControl v2 unit tests --- .../custom-select-control-v2/legacy-component/test/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8696da43aa3a9..48155407fd2f8 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 @@ -560,7 +560,7 @@ describe.each( [ // Assert that the option element does not have the custom attributes expect( optionElement ).not.toHaveAttribute( 'customPropFoo' ); - expect( optionElement ).not.toHaveAttribute( 'customPropBar' ); + expect( optionElement ).not.toHaveAttribute( 'aria-label' ); await click( optionElement ); From dc775206ccffe58e85e09ab35f4f00990f77082c Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 12:50:38 +0200 Subject: [PATCH 12/18] Update unit tests --- .../legacy-component/test/index.tsx | 98 +++++++++++-------- .../src/custom-select-control/test/index.js | 93 +++++++++++------- 2 files changed, 114 insertions(+), 77 deletions(-) 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 48155407fd2f8..6885205b03fd3 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 @@ -46,6 +46,16 @@ const legacyProps = { name: 'aquamarine', style: customStyles, }, + { + key: 'color3', + name: 'tomato (with custom props)', + className: customClassName, + style: customStyles, + // try passing a valid HTML attribute + 'aria-label': 'test label', + // try adding a custom prop + customPropFoo: 'foo', + }, ], }; @@ -289,8 +299,7 @@ describe.each( [ expect.objectContaining( { inputValue: '', isOpen: false, - // TODO: key should be different — this is a known bug and will be fixed - selectedItem: { key: 'violets', name: 'violets' }, + selectedItem: { key: 'flower1', name: 'violets' }, type: '', } ) ); @@ -333,8 +342,7 @@ describe.each( [ 1, expect.objectContaining( { selectedItem: expect.objectContaining( { - // TODO: key should be different — this is a known bug and will be fixed - key: 'violets', + key: 'flower1', name: 'violets', } ), } ) @@ -347,14 +355,56 @@ describe.each( [ 2, expect.objectContaining( { selectedItem: expect.objectContaining( { - // TODO: key should be different — this is a known bug and will be fixed - key: 'poppy', + key: 'flower3', name: 'poppy', } ), } ) ); } ); + it( 'Should accept and pass arbitrary properties to the selectedItem object in the onChange callback, but without applying them to the DOM elements apart from style and classname', async () => { + const onChangeMock = jest.fn(); + + render( ); + + const currentSelectedItem = screen.getByRole( 'combobox', { + expanded: false, + } ); + + await click( currentSelectedItem ); + + const optionWithCustomAttributes = screen.getByRole( 'option', { + name: 'tomato (with custom props)', + } ); + + // Assert that the option element does not have the custom attributes + expect( optionWithCustomAttributes ).not.toHaveAttribute( + 'customPropFoo' + ); + expect( optionWithCustomAttributes ).not.toHaveAttribute( + 'aria-label' + ); + + 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 ).toHaveBeenCalledWith( + expect.objectContaining( { + selectedItem: expect.objectContaining( { + key: 'color3', + name: 'tomato (with custom props)', + className: customClassName, + style: customStyles, + 'aria-label': 'test label', + customPropFoo: 'foo', + } ), + } ) + ); + } ); + describe( 'Keyboard behavior and accessibility', () => { // skip reason: legacy v2 doesn't currently implement this behavior it.skip( 'Captures the keypress event and does not let it propagate', async () => { @@ -537,40 +587,4 @@ describe.each( [ expect( onBlurMock ).toHaveBeenCalledTimes( 1 ); } ); } ); - - // V1 styles items via a `style` or `className` metadata property in the option item object. Some consumers still expect it, e.g: - // - https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/font-appearance-control/index.js#L216 - // Besides that, the `option` prop is documented as havin the type: - // - `Array<{ key: String, name: String, style: ?{}, className: ?String, ...rest }>` - // Notice the `...test` there. We should keep supporting the arbitrary props like this. - it( 'Should return style and custom metadata as part of the selected option from onChange', async () => { - const mockOnChange = jest.fn(); - - render( ); - - await click( - screen.getByRole( 'combobox', { - expanded: false, - } ) - ); - - const optionElement = screen.getByRole( 'option', { - name: 'aquarela', - } ); - - // Assert that the option element does not have the custom attributes - expect( optionElement ).not.toHaveAttribute( 'customPropFoo' ); - expect( optionElement ).not.toHaveAttribute( 'aria-label' ); - - await click( optionElement ); - - expect( mockOnChange ).toHaveBeenCalledWith( - expect.objectContaining( { - selectedItem: expect.objectContaining( { - className: customClassName, - style: customStyles, - } ), - } ) - ); - } ); } ); diff --git a/packages/components/src/custom-select-control/test/index.js b/packages/components/src/custom-select-control/test/index.js index 63172f1e8ad32..550607a024906 100644 --- a/packages/components/src/custom-select-control/test/index.js +++ b/packages/components/src/custom-select-control/test/index.js @@ -46,6 +46,16 @@ const props = { name: 'aquamarine', style: customStyles, }, + { + key: 'color3', + name: 'tomato (with custom props)', + className: customClassName, + style: customStyles, + // try passing a valid HTML attribute + 'aria-label': 'test label', + // try adding a custom prop + customPropFoo: 'foo', + }, ], }; @@ -247,56 +257,28 @@ describe.each( [ ).toHaveTextContent( 'Hint' ); } ); - it( 'Should accept and pass arbitrary properties to the selectedItem object in the onChange callback', async () => { + it( 'shows selected hint in list of options when added, regardless of __experimentalShowSelectedHint prop', async () => { const user = userEvent.setup(); - const onChangeMock = jest.fn(); render( ); - const currentSelectedItem = screen.getByRole( 'button', { - expanded: false, - } ); - - await user.click( currentSelectedItem ); await user.click( - screen.getByRole( 'option', { name: 'Custom Option' } ) + screen.getByRole( 'button', { name: 'Custom select' } ) ); - expect( onChangeMock ).toHaveBeenCalledTimes( 1 ); - expect( onChangeMock ).toHaveBeenCalledWith( - expect.objectContaining( { - selectedItem: expect.objectContaining( { - key: 'custom', - name: 'Custom Option', - className: 'custom-class-name', - customProp1: 'value1', - customProp2: 42, - style: { - backgroundColor: 'rgb(127, 255, 212)', - rotate: '13deg', - }, - } ), - } ) - ); + expect( screen.getByRole( 'option', { name: /hint/i } ) ).toBeVisible(); } ); it( 'Should return object onChange', async () => { @@ -374,6 +356,47 @@ describe.each( [ ); } ); + it( 'Should accept and pass arbitrary properties to the selectedItem object in the onChange callback, but without applying them to the DOM elements apart from style and classname', async () => { + const user = userEvent.setup(); + const onChangeMock = jest.fn(); + + render( ); + + const currentSelectedItem = screen.getByRole( 'button', { + expanded: false, + } ); + + await user.click( currentSelectedItem ); + + const optionWithCustomAttributes = screen.getByRole( 'option', { + name: 'tomato (with custom props)', + } ); + + // Assert that the option element does not have the custom attributes + expect( optionWithCustomAttributes ).not.toHaveAttribute( + 'customPropFoo' + ); + expect( optionWithCustomAttributes ).not.toHaveAttribute( + 'aria-label' + ); + + await user.click( optionWithCustomAttributes ); + + expect( onChangeMock ).toHaveBeenCalledTimes( 1 ); + expect( onChangeMock ).toHaveBeenCalledWith( + expect.objectContaining( { + selectedItem: expect.objectContaining( { + key: 'color3', + name: 'tomato (with custom props)', + className: customClassName, + style: customStyles, + 'aria-label': 'test label', + customPropFoo: 'foo', + } ), + } ) + ); + } ); + describe( 'Keyboard behavior and accessibility', () => { it( 'Captures the keypress event and does not let it propagate', async () => { const user = userEvent.setup(); From 192ac71d5006ae30055cff063a1870fe385c493a Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 17:51:40 +0200 Subject: [PATCH 13/18] Remove stable export --- packages/components/src/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/components/src/index.ts b/packages/components/src/index.ts index 1ec3ae567108d..f3643a1499a02 100644 --- a/packages/components/src/index.ts +++ b/packages/components/src/index.ts @@ -64,7 +64,6 @@ export { } from './composite'; export { ConfirmDialog as __experimentalConfirmDialog } from './confirm-dialog'; export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control'; -export { default as CustomSelectControlV2 } from './custom-select-control-v2'; export { default as Dashicon } from './dashicon'; export { default as DateTimePicker, DatePicker, TimePicker } from './date-time'; export { default as __experimentalDimensionControl } from './dimension-control'; From c6ee50800de37b30bf1be4c900008ef6d8746719 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 17:55:05 +0200 Subject: [PATCH 14/18] Export v2 legacy adapter as private API --- packages/components/src/private-apis.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/components/src/private-apis.ts b/packages/components/src/private-apis.ts index f55373664efff..a8bacd5954bb7 100644 --- a/packages/components/src/private-apis.ts +++ b/packages/components/src/private-apis.ts @@ -14,6 +14,7 @@ import { useCompositeStore as useCompositeStoreV2, } from './composite/v2'; import { default as CustomSelectControl } from './custom-select-control'; +import { default as CustomSelectControlV2LegacyAdapter } from './custom-select-control-v2/legacy-component'; import { positionToPlacement as __experimentalPopoverLegacyPositionToPlacement } from './popover/utils'; import { createPrivateSlotFill } from './slot-fill'; import { @@ -40,6 +41,7 @@ lock( privateApis, { CompositeRowV2, useCompositeStoreV2, CustomSelectControl, + CustomSelectControlV2LegacyAdapter, __experimentalPopoverLegacyPositionToPlacement, createPrivateSlotFill, ComponentsContext, From 8d0d0199277ed5c6ed97bee530000f2cc25f6194 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 18:00:10 +0200 Subject: [PATCH 15/18] CHANGELOG --- packages/components/CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index bc4c7da0734d1..9a595827ffb0c 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -4,11 +4,12 @@ ### Enhancements -- DropZone: rewrite animation without depending on framer-motion. ([#62044](https://github.com/WordPress/gutenberg/pull/62044)) +- `DropZone`: rewrite animation without depending on framer-motion. ([#62044](https://github.com/WordPress/gutenberg/pull/62044)) ### Internal -- CustomSelectControl: align unit tests for v1 and legacy v2 versions. ([#62706](https://github.com/WordPress/gutenberg/pull/62706)) +- `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)) ## 28.1.0 (2024-06-15) From ecf390daf9ef25a49f2e589e7628e5b82fbfc6fa Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 20 Jun 2024 18:59:07 +0200 Subject: [PATCH 16/18] Allow v1 Storybook to log onChange args in the actions tab for easier debugging --- .../stories/index.story.tsx | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/packages/components/src/custom-select-control/stories/index.story.tsx b/packages/components/src/custom-select-control/stories/index.story.tsx index a27ad6b3894b6..8ff9a023c5821 100644 --- a/packages/components/src/custom-select-control/stories/index.story.tsx +++ b/packages/components/src/custom-select-control/stories/index.story.tsx @@ -3,6 +3,11 @@ */ import type { StoryFn } from '@storybook/react'; +/** + * WordPress dependencies + */ +import { useState } from '@wordpress/element'; + /** * Internal dependencies */ @@ -20,10 +25,34 @@ export default { type: 'radio', }, }, + onChange: { control: { type: null } }, + value: { control: { type: null } }, + }, + parameters: { + actions: { argTypesRegex: '^on.*' }, }, }; -export const Default: StoryFn = CustomSelectControl.bind( {} ); +const Template: StoryFn< typeof CustomSelectControl > = ( props ) => { + const [ value, setValue ] = useState( props.options[ 0 ] ); + + const onChange: React.ComponentProps< + typeof CustomSelectControl + >[ 'onChange' ] = ( changeObject: { selectedItem: any } ) => { + setValue( changeObject.selectedItem ); + props.onChange?.( changeObject ); + }; + + return ( + + ); +}; + +export const Default: StoryFn = Template.bind( {} ); Default.args = { label: 'Label', options: [ @@ -51,7 +80,7 @@ Default.args = { ], }; -export const WithLongLabels: StoryFn = CustomSelectControl.bind( {} ); +export const WithLongLabels: StoryFn = Template.bind( {} ); WithLongLabels.args = { ...Default.args, options: [ @@ -70,7 +99,7 @@ WithLongLabels.args = { ], }; -export const WithHints: StoryFn = CustomSelectControl.bind( {} ); +export const WithHints: StoryFn = Template.bind( {} ); WithHints.args = { ...Default.args, options: [ From bd0010f5d141b24eca676608a4a2937c08da8920 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 21 Jun 2024 15:23:32 +0200 Subject: [PATCH 17/18] Do not export legacy adapter as private API just yet --- packages/components/src/private-apis.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/components/src/private-apis.ts b/packages/components/src/private-apis.ts index a8bacd5954bb7..f55373664efff 100644 --- a/packages/components/src/private-apis.ts +++ b/packages/components/src/private-apis.ts @@ -14,7 +14,6 @@ import { useCompositeStore as useCompositeStoreV2, } from './composite/v2'; import { default as CustomSelectControl } from './custom-select-control'; -import { default as CustomSelectControlV2LegacyAdapter } from './custom-select-control-v2/legacy-component'; import { positionToPlacement as __experimentalPopoverLegacyPositionToPlacement } from './popover/utils'; import { createPrivateSlotFill } from './slot-fill'; import { @@ -41,7 +40,6 @@ lock( privateApis, { CompositeRowV2, useCompositeStoreV2, CustomSelectControl, - CustomSelectControlV2LegacyAdapter, __experimentalPopoverLegacyPositionToPlacement, createPrivateSlotFill, ComponentsContext, From c95b5c1d7c567227540725dc1767ec126f04007e Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 21 Jun 2024 15:25:24 +0200 Subject: [PATCH 18/18] Shorter test name --- .../custom-select-control-v2/legacy-component/test/index.tsx | 2 +- packages/components/src/custom-select-control/test/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 6885205b03fd3..06af287a14236 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 @@ -362,7 +362,7 @@ describe.each( [ ); } ); - it( 'Should accept and pass arbitrary properties to the selectedItem object in the onChange callback, but without applying them to the DOM elements apart from style and classname', async () => { + it( "Should pass arbitrary props to onChange's selectedItem, but apply only style and className to DOM elements", async () => { const onChangeMock = jest.fn(); render( ); diff --git a/packages/components/src/custom-select-control/test/index.js b/packages/components/src/custom-select-control/test/index.js index 550607a024906..2eb855c15b51f 100644 --- a/packages/components/src/custom-select-control/test/index.js +++ b/packages/components/src/custom-select-control/test/index.js @@ -356,7 +356,7 @@ describe.each( [ ); } ); - it( 'Should accept and pass arbitrary properties to the selectedItem object in the onChange callback, but without applying them to the DOM elements apart from style and classname', async () => { + it( "Should pass arbitrary props to onChange's selectedItem, but apply only style and className to DOM elements", async () => { const user = userEvent.setup(); const onChangeMock = jest.fn();