Skip to content

Commit

Permalink
CustomSelectControlV2 legacy adapter: fix handling of options extra a…
Browse files Browse the repository at this point in the history 
…ttributes (#62255)

* 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

* Remove unrelated test for this part

* Remove debug code

* Add test to V1 to confirm it supports passing custom properties as part of an option

* 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)

* Type `LegacyOption` to support arbitrary attributes

* Remove re-export for the adapter for now

* Remove non-nullable operator in favour of an early return

* Remove extra space

* Set default false value via prop destructuring

* Use a valid HTML attribute in CustomSelectControl v2 unit tests

* Update unit tests

* Remove stable export

* Export v2 legacy adapter as private API

* CHANGELOG

* Allow v1 Storybook to log onChange args in the actions tab for easier debugging

* Do not export legacy adapter as private API just yet

* Shorter test name

---------

Co-authored-by: fullofcaffeine <fullofcaffeine@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
  • Loading branch information
@fullofcaffeine @ciampo @tyxla
4 people authored Jun 21, 2024
1 parent 26b7a12 commit b0a160b
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 19 deletions.
5 changes: 3 additions & 2 deletions packages/components/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
18 changes: 10 additions & 8 deletions packages/components/src/custom-select-control-v2/legacy-component/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import * as Styled from '../styles';

function CustomSelectControl( props: LegacyCustomSelectProps ) {
const {
__experimentalShowSelectedHint,
__experimentalShowSelectedHint = false,
__next40pxDefaultSize = false,
describedBy,
options,
Expand All @@ -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;
}

Expand All @@ -42,18 +46,15 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
),
inputValue: '',
isOpen: state.open,
selectedItem: {
name: nextValue as string,
key: nextValue as string,
},
selectedItem: nextOption,
type: '',
};
onChange( changeObject );
},
} );

const children = options.map(
( { name, key, __experimentalHint, ...rest } ) => {
( { name, key, __experimentalHint, style, className } ) => {
const withHint = (
<Styled.WithHintWrapper>
<span>{ name }</span>
Expand All @@ -68,7 +69,8 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
key={ key }
value={ name }
children={ __experimentalHint ? withHint : name }
{ ...rest }
style={ style }
className={ className }
/>
);
}
Expand Down
62 changes: 56 additions & 6 deletions packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
],
};

Expand Down Expand Up @@ -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: '',
} )
);
Expand Down Expand Up @@ -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',
} ),
} )
Expand All @@ -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 pass arbitrary props to onChange's selectedItem, but apply only style and className to DOM elements", async () => {
const onChangeMock = jest.fn();

render( <Component { ...legacyProps } onChange={ onChangeMock } /> );

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 () => {
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/custom-select-control-v2/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ type LegacyOption = {
style?: React.CSSProperties;
className?: string;
__experimentalHint?: string;
[ key: string ]: any;
};

/**
Expand Down
35 changes: 32 additions & 3 deletions packages/components/src/custom-select-control/stories/index.story.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
*/
import type { StoryFn } from '@storybook/react';

/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
Expand All @@ -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 (
<CustomSelectControl
{ ...props }
onChange={ onChange }
value={ value }
/>
);
};

export const Default: StoryFn = Template.bind( {} );
Default.args = {
label: 'Label',
options: [
Expand Down Expand Up @@ -51,7 +80,7 @@ Default.args = {
],
};

export const WithLongLabels: StoryFn = CustomSelectControl.bind( {} );
export const WithLongLabels: StoryFn = Template.bind( {} );
WithLongLabels.args = {
...Default.args,
options: [
Expand All @@ -70,7 +99,7 @@ WithLongLabels.args = {
],
};

export const WithHints: StoryFn = CustomSelectControl.bind( {} );
export const WithHints: StoryFn = Template.bind( {} );
WithHints.args = {
...Default.args,
options: [
Expand Down
51 changes: 51 additions & 0 deletions packages/components/src/custom-select-control/test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
],
};

Expand Down Expand Up @@ -346,6 +356,47 @@ describe.each( [
);
} );

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();

render( <Component { ...props } onChange={ onChangeMock } /> );

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();
Expand Down

1 comment on commit b0a160b

@github-actions
Copy link

Choose a reason for hiding this comment

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

Flaky tests detected in b0a160b.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9614847257
📝 Reported issues:

Please sign in to comment.