-
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
CustomSelect: Adapt component for legacy props #57902
Conversation
const changeObject = { | ||
selectedItem: { | ||
// value will always be a string for the legacy component | ||
name: value as string, | ||
key: state.activeId as string, | ||
}, | ||
highlightedIndex: state.renderedItems.findIndex( | ||
( item ) => item.value === value | ||
), | ||
isOpen: state.open, | ||
}; | ||
props.onChange( changeObject ); |
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.
Just to note, I'm still looking into the onChange return object. I've implemented the suggestion by @diegohaz from an earlier PR: #55234 (comment), minus the type
and inputValue
. I'm looking to see if it makes sense to include these / how we could implement them.
The value of type
is the interaction method used to make a selection, i.e. __item_click__
, __menu_keydown_enter__
The value of inputValue
is the first character entered via keydown to make a selection (with type
returning __togglebutton_keydown_character__
). If keydown isn't used to make a selection, then it returns an empty string.
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 value of
type
is the interaction method used to make a selection, i.e.__item_click__
,__menu_keydown_enter__
[...]
The value ofinputValue
is the first character entered via keydown to make a selection (withtype
returning__togglebutton_keydown_character__
). If keydown isn't used to make a selection, then it returns an empty string.
Mhh, I'm not sure we can reliably access those pieces of information. Maybe Diego can advise.
In the meantime, we can give them a fixed ''
value to at least make sure that if anyone is accessing those properties, they are defined.
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.
I'd keep the type
as __item_click__
by default for simplicity's sake. If anyone depends on these values, it's likely the type they're handling as standard.
You could get this value by listening to DOM events and storing the information in a ref for subsequent use in Ariakit's setValue
prop. But honestly, I wouldn't sweat it. It's not even mentioned in the Downshift docs. It doesn't seem to be part of their public API.
Edit: I was wrong. It is part of their public API (see stateChangeTypes
). Still, if these constants weren't exposed by WordPress components, I'd avoid re-implementing this logic.
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.
This is looking promising!
I know that it's still a draft, but I left a few initial comments that are hopefully going to help move the work forward 🤞
packages/components/src/custom-select-control-v2/stories/legacy.story.tsx
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-adapter.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/stories/legacy.story.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/stories/legacy.story.tsx
Outdated
Show resolved
Hide resolved
const changeObject = { | ||
selectedItem: { | ||
// value will always be a string for the legacy component | ||
name: value as string, | ||
key: state.activeId as string, | ||
}, | ||
highlightedIndex: state.renderedItems.findIndex( | ||
( item ) => item.value === value | ||
), | ||
isOpen: state.open, | ||
}; | ||
props.onChange( changeObject ); |
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 value of
type
is the interaction method used to make a selection, i.e.__item_click__
,__menu_keydown_enter__
[...]
The value ofinputValue
is the first character entered via keydown to make a selection (withtype
returning__togglebutton_keydown_character__
). If keydown isn't used to make a selection, then it returns an empty string.
Mhh, I'm not sure we can reliably access those pieces of information. Maybe Diego can advise.
In the meantime, we can give them a fixed ''
value to at least make sure that if anyone is accessing those properties, they are defined.
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/stories/default.story.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
d042c2f
to
007aba0
Compare
Flaky tests detected in 3a2be41. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7850850332
|
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
0d1e182
to
63f5d28
Compare
@@ -22,7 +27,13 @@ export type CustomSelectProps = { | |||
* An optional default value for the control. If left `undefined`, the first | |||
* non-disabled item will be used. | |||
*/ | |||
defaultValue?: string | string[]; | |||
defaultValue: string | string[]; |
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.
When adding unmountOnHide
, the initial selected value was an empty string because it no longer had access to the items through the store (see details here: ariakit/ariakit#3374 (comment))
I'm thinking it might be cleanest to require a defaultValue
(for the new version, the legacy would use the hardcoded default to behave the same), as it's just a string for the consumer to implement.
Otherwise, we'd have to map over the children to find a default value. The problem here is that the children could be a fragment or a div or anything that the consumer implements. So it seems like it could get a bit messy to extract a value.
It's also likely I'm overcomplicating it and there is an easy way to get the value. I'm happy to hear if there is a better solution. 🙂
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.
Is defaultValue
still required if the value
prop is provided?
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.
As Diego suggests, defaultValue
shouldn't be required when value
is provided — it would actually cause more confusion, since defaultValue
is used to provide an initial value to the component when used as uncontrolled, while value
is used to set the component's value in a controlled mode.
Otherwise, we'd have to map over the children to find a default value. The problem here is that the children could be a fragment or a div or anything that the consumer implements
If we expect children to be instances of CustomSelectControlItem
, then we could read the value
prop, which is required on those components, and assume as a default value the first defined value
that we find when mapping over the children.
Would that work?
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.
Is there a way we can access the value
prop without mapping through the children?
That is the only way I am aware of. Other than doing something that might be considered strange and passing it from the child to parent through context.
If we map though the children to find the value
, then we're back to the same problem I shared earlier where we might not know how many layers deep the value is. For example, if they are wrapped in a div
:
Unless we have a recursive function but that's where I thought it was getting messy or that I was overcomplicating it.
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.
Yeah, it's tricky. If the CustomSelectControlItem
component gets abstracted into another custom component, you won't have access to it, even with recursion. It seems odd to impose such artificial restrictions on JSX, where users expect certain features to work seamlessly. It's like trying to use JSX as a DSL for an array of objects, when simply using the array of objects might be the answer.
I'd suggest that either defaultValue
or value
should be required. There are ways to express this in TypeScript, but a runtime warning might be necessary because the TypeScript error may not be very clear.
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.
This might be a strange idea, but after seeing Lena's work I thought maybe we could add another hack for unmountOnHide
😄
If we mount and then unmount it on the initial render, then we get the first item as the defaultValue without explicitly setting it, like before:
gutenberg/packages/components/src/custom-select-control-v2/custom-select.tsx
Lines 112 to 116 in e3c328f
const [ unmountOnHide, setUnmountOnHide ] = useState( false ); | |
useEffect( () => { | |
setUnmountOnHide( true ); | |
}, [] ); |
Could this be a potential solution?
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.
but the test for this behavior is still failing:
From personal experience, user-event
may cause some incompatibilities with ariakit-based components, especially when we make some async changes like in the suggestion above. Maybe switching to @ariakit/test
fixes it?
Could this be a potential solution?
It could work, as an additional hack. I've updated the example and it seems to work as expected. It may have some performance ripercussion, though — so we could go with this implementation at first (which allows us to keep defaultValue
as optional), and only switch to requiring defaultValue
as a fallback? What do folks think?
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.
Maybe switching to @ariakit/test fixes it?
It does! I just thought we might want to keep the tests as close to the original CustomSelectControl
as possible, so I planned to move to ariakit/test
after. But since the tests are the same aside from the interaction method, maybe it will be okay to do that 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.
If we mount and then unmount it on the initial render
I feel like the initial render is exactly the most important time to reap the performance benefits of unmountOnHide
, so I'm not sure it makes sense to mount on initial render but hide it after.
So going back to basics, if we don't already have proof that just keeping unmountOnHide={ false }
all the time has a perceivable performance hit to the overall Gutenberg app, I'd even be fine with not messing with that prop at all for the first iteration. I have to imagine that there are much fewer CustomSelectControls in any given view than a generic Tooltip, for example.
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.
So, we remove unmountOnHide
and all of the related workarounds? 🙈 If I understood correctly, then that is done through: 6baa642
Just to note: the tests will pass using @testing-library/user-event
or with @ariakit/test
. I believe the former is faster, but I think we will likely need to move to @ariakit/test
when integrating the new tests (#58583) — based on seeing flaky test results when merging the new and legacy tests in another version of the legacy adapter: #57000
We could revert to RTL, merge this PR, and then merge the tests/change to @ariakit/test
if needed in a follow-up. Or we could keep it with @ariakit/test
, and I can merge the new tests into this.
Any preferences?
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
6a07814
to
4d6819b
Compare
packages/components/src/custom-select-control-v2/custom-select-item.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/custom-select-item.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/custom-select.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/custom-select.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-adapter.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-adapter.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
5babba6
to
e3c328f
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
packages/components/src/custom-select-control-v2/custom-select.tsx
Outdated
Show resolved
Hide resolved
@@ -22,7 +27,13 @@ export type CustomSelectProps = { | |||
* An optional default value for the control. If left `undefined`, the first | |||
* non-disabled item will be used. | |||
*/ | |||
defaultValue?: string | string[]; | |||
defaultValue: string | string[]; |
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.
With respect to the initial value, it should be determined by the consumer by passing either the value prop (controlled mode) or the defaultValue prop (uncontrolled mode)
Would that involve typing so only one is required when the other isn't present? Something like:
type CustomSelectModeProps =
| {
/**
* A default value to show as the currently selected item. Not required when adding a
* value to use component in controlled mode.
*/
defaultValue: string | string[];
value?: string | string[];
}
| {
defaultValue?: string | string[];
/**
* Can be used to externally control the value of the control.
*/
value: string | string[];
};
There are still some issues with the defaultValue
for the legacy component. When a default value is provided, tests are failing, but if we don't pass one, we have the empty initial state due to the unmountOnHide
prop. So I'm still looking into that
@@ -22,7 +27,13 @@ export type CustomSelectProps = { | |||
* An optional default value for the control. If left `undefined`, the first | |||
* non-disabled item will be used. | |||
*/ | |||
defaultValue?: string | string[]; | |||
defaultValue: string | string[]; |
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.
however, Lena suggested a little hack which seems to work: set unmountOnHide to false when focusing the button that is triggering the dropdown (see example)
This suggestion worked well in combination with these additional props . When testing in SB, it works as expected, but the test for this behavior is still failing:
it( 'Can change selection with a focused input and closed dropdown if typed characters match an option', async () => { |
@@ -22,7 +27,13 @@ export type CustomSelectProps = { | |||
* An optional default value for the control. If left `undefined`, the first | |||
* non-disabled item will be used. | |||
*/ | |||
defaultValue?: string | string[]; | |||
defaultValue: string | string[]; |
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.
This might be a strange idea, but after seeing Lena's work I thought maybe we could add another hack for unmountOnHide
😄
If we mount and then unmount it on the initial render, then we get the first item as the defaultValue without explicitly setting it, like before:
gutenberg/packages/components/src/custom-select-control-v2/custom-select.tsx
Lines 112 to 116 in e3c328f
const [ unmountOnHide, setUnmountOnHide ] = useState( false ); | |
useEffect( () => { | |
setUnmountOnHide( true ); | |
}, [] ); |
Could this be a potential solution?
packages/components/src/custom-select-control-v2/legacy-adapter.tsx
Outdated
Show resolved
Hide resolved
If this solution is something we can do to handle
And then, of course, smoke-testing it in the editor once the above is resolved. |
794fcd0
to
c73ce18
Compare
I've combined the tests from #58583 in this commit: c73ce18 It's super verbose, with some tests being in both the suites for the legacy and default sections. But with the uncontrolled/controlled sections, I wasn't sure how to do it without overcomplicating it / making it very confusing. I've let @mirka know about this, as she'll be taking this over this PR at this stage. 🙇♀️ |
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.
I did a bit of overall clean up in terms of naming, documentation, and styles.
Since this PR has been running long and over-reviewed at this point, I'd like to merge now and iterate in more smaller scopes.
I identified these remaining tasks, and added them to the tracking issue (#55023):
- Remove legacy adapter layer (make two separate exports)
- Rename to
CustomSelectControl
andCustomSelectControl.Item
- Try to make the TS types for the single/multiple selection modes be more ergonomic
- Styling issues
- Focus styles are incorrect
- Needs a utility padding wrapper like
InputControlPrefixWrapper
- Line-height is wrong
What?
Another option to replace CustomSelectControl with the new ariakit verison without breaking changes. This started in #57000, which was using a similiar approach to ColorPicker's legacy adapter. It was getting a bit complicated, so this PR ius based on another approach shared by @ciampo. This has the same goal but aims to be less convoluted.
Why?
To transition away from using CustomSelectControl to the new version for more flexibility.
How?
No changes were made to CustomSelectItem, aside from moving it to a separate file.
The same for CustomSelect, and the store has been moved outside of the component and instead is forwarded the store.
In this version, there are two components, one for the legacy and another for the new/soon-to-be-default version. There is a check to see which props have been passed through. If it's the legacy, the props are translated to the new version of the component.
This also adds the same tests from CustomSelectControl. The only changes made were to the role which is now combobox instead of button, and the tests for the custom event handlers have been removed.
To Do
unmountOnHide
- add to both or just legacy? Related: Select:defaultValue
doesn't behave as intended when usingunmountOnHide
ariakit/ariakit#3374Legacy
arrowcheckmark sizeNew
Testing Instructions
run npm storybook:dev