-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Components: Replace/old custom select control with v2 legacy adapter #61272
Closed
fullofcaffeine
wants to merge
19
commits into
trunk
from
replace/old-custom-select-control-with-v2-legacy-adapter
Closed
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
cc17f9f
Replace old `CustomSelectControl` select control with the V2 legacy a…
fullofcaffeine 33bb57d
Fix option handling logic for legacy adater and update test to reflec…
fullofcaffeine f2c059c
Remove debug code
fullofcaffeine 0e83151
Debug why selectedItem is not selected on mount
fullofcaffeine db8379a
Ensure correct item is selected upon mount based on the value passed …
fullofcaffeine 751b295
Add new test case to make sure initial passed value is selected upon …
fullofcaffeine d18f88d
CSS tweaks
fullofcaffeine 218cc67
More CSS adjustments to make the component behave closer to classic
fullofcaffeine ee619f0
Remove unused prop
fullofcaffeine ac751d9
Fix experimentalHint custom styling on legacy adapter
fullofcaffeine 0db8e28
Fix width of popover not always matching the width of the select
fullofcaffeine 98a0fc4
Improve export name
fullofcaffeine e88b0c4
Fix label margin not applying in certain contexts
fullofcaffeine d2cfd1d
Add a wrapper around the select as it the main container needs to hav…
fullofcaffeine e47adcf
Fix withHint handling
fullofcaffeine e38a4c8
Fix tests
fullofcaffeine 17f6bbd
Revert accidental change
fullofcaffeine cd537af
Make the select item text non-selectable
fullofcaffeine fd5c051
Merge branch 'trunk' into replace/old-custom-select-control-with-v2-l…
fullofcaffeine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,11 @@ import { useState } from '@wordpress/element'; | |
*/ | ||
import UncontrolledCustomSelectControl from '..'; | ||
|
||
const customClass = 'amber-skies'; | ||
const className = 'amber-skies'; | ||
const style = { | ||
backgroundColor: 'rgb(127, 255, 212)', | ||
rotate: '13deg', | ||
}; | ||
|
||
const legacyProps = { | ||
label: 'label!', | ||
|
@@ -26,7 +30,7 @@ const legacyProps = { | |
{ | ||
key: 'flower2', | ||
name: 'crimson clover', | ||
className: customClass, | ||
className, | ||
}, | ||
{ | ||
key: 'flower3', | ||
|
@@ -35,15 +39,18 @@ const legacyProps = { | |
{ | ||
key: 'color1', | ||
name: 'amber', | ||
className: customClass, | ||
className, | ||
}, | ||
{ | ||
key: 'color2', | ||
name: 'aquamarine', | ||
style: { | ||
backgroundColor: 'rgb(127, 255, 212)', | ||
rotate: '13deg', | ||
}, | ||
style, | ||
}, | ||
{ | ||
key: 'aquarela-key', | ||
name: 'aquarela', | ||
className, | ||
style, | ||
}, | ||
], | ||
}; | ||
|
@@ -53,7 +60,9 @@ const ControlledCustomSelectControl = ( { | |
onChange, | ||
...restProps | ||
}: React.ComponentProps< typeof UncontrolledCustomSelectControl > ) => { | ||
const { value: overrideValue } = restProps; | ||
const [ value, setValue ] = useState( options[ 0 ] ); | ||
const initialValue = overrideValue ?? value; | ||
return ( | ||
<UncontrolledCustomSelectControl | ||
{ ...restProps } | ||
|
@@ -63,7 +72,7 @@ const ControlledCustomSelectControl = ( { | |
setValue( args.selectedItem ); | ||
} } | ||
value={ options.find( | ||
( option: any ) => option.key === value.key | ||
( option: any ) => option.key === initialValue.key | ||
) } | ||
/> | ||
); | ||
|
@@ -148,7 +157,7 @@ describe.each( [ | |
// assert against filtered array | ||
itemsWithClass.map( ( { name } ) => | ||
expect( screen.getByRole( 'option', { name } ) ).toHaveClass( | ||
customClass | ||
className | ||
) | ||
); | ||
|
||
|
@@ -160,7 +169,7 @@ describe.each( [ | |
// assert against filtered array | ||
itemsWithoutClass.map( ( { name } ) => | ||
expect( screen.getByRole( 'option', { name } ) ).not.toHaveClass( | ||
customClass | ||
className | ||
) | ||
); | ||
} ); | ||
|
@@ -286,7 +295,7 @@ describe.each( [ | |
expect.objectContaining( { | ||
inputValue: '', | ||
isOpen: false, | ||
selectedItem: { key: 'violets', name: 'violets' }, | ||
selectedItem: { key: 'flower1', name: 'violets' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact it had the same |
||
type: '', | ||
} ) | ||
); | ||
|
@@ -302,6 +311,7 @@ describe.each( [ | |
expect.objectContaining( { | ||
inputValue: '', | ||
isOpen: false, | ||
|
||
selectedItem: expect.objectContaining( { | ||
name: 'aquamarine', | ||
} ), | ||
|
@@ -328,7 +338,7 @@ describe.each( [ | |
await type( 'p' ); | ||
await press.Enter(); | ||
|
||
expect( mockOnChange ).toHaveReturnedWith( 'poppy' ); | ||
expect( mockOnChange ).toHaveReturnedWith( 'flower1' ); | ||
} ); | ||
|
||
describe( 'Keyboard behavior and accessibility', () => { | ||
|
@@ -457,4 +467,57 @@ describe.each( [ | |
).toBeVisible(); | ||
} ); | ||
} ); | ||
|
||
// 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( <Component { ...legacyProps } onChange={ mockOnChange } /> ); | ||
|
||
await click( | ||
screen.getByRole( 'combobox', { | ||
expanded: false, | ||
} ) | ||
); | ||
|
||
await click( | ||
screen.getByRole( 'option', { | ||
name: 'aquarela', | ||
} ) | ||
); | ||
|
||
expect( mockOnChange ).toHaveBeenCalledWith( | ||
expect.objectContaining( { | ||
selectedItem: expect.objectContaining( { | ||
className, | ||
style, | ||
} ), | ||
} ) | ||
); | ||
} ); | ||
|
||
it( 'Should display the initial value passed as the selected value', async () => { | ||
const initialSelectedItem = legacyProps.options[ 5 ]; | ||
|
||
const testProps = { | ||
...legacyProps, | ||
value: initialSelectedItem, | ||
}; | ||
|
||
render( <Component { ...testProps } /> ); | ||
|
||
const currentSelectedItem = await screen.findByRole( 'combobox', { | ||
expanded: false, | ||
} ); | ||
|
||
// Verify the initial selected value | ||
expect( currentSelectedItem ).toHaveTextContent( 'aquarela' ); | ||
} ); | ||
} ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old object here didn't include any custom attributes that might was part of the original options array passed to the component. Not sure we should instead pass these values through the Ariakit's API (e.g use
value
to store thestyle
data for example), but selecting the option by name is a (hacky) workaround that works for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @mirka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, after spending more time with the codebase for V2/LegacyAdapter and the V1 component, I think I grasped what was wrong here. There were two issues:
selectedItem
object had itsname
andkey
both set to thenextValue
argument passed to thesetValue
callback, resulting in both having thename
attribute from the options array. This didn't look right.style
and/orclassName
) that was considered to be implicitly available in the item passed to theonChange
callback in V1, which some consumers expect.I've fixed both in 56461bd, but I'm wondering if this is the right approach and if should we keep the same API in V2 (as in keep the style metadata in the item passed to
onChange
?), or should we refactor the consumers to not rely on this self-contained data and instead move it to a store somewhere else? Of course, this would be work to be done in follow-up PRs.See the test / comment here (I'll simplify or remove that comment before merging).