Add async select control #34949
Add async select control #34949
Conversation
github-actions
bot
added
the
package: @woocommerce/components
Test Results SummaryCommit SHA: e7a3277
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
louwie17
force-pushed
the
add/34331_add_async_select_control
branch
from
a5ed9a7 to
00277fd
Compare
louwie17
force-pushed
the
add/34331_add_async_select_control
branch
from
00277fd to
e7a3277
Compare
| onInputValueChange: ( { inputValue: value, ...changes } ) => { | ||
| if ( value !== undefined ) { | ||
| setInputValue( value ); | ||
| onInputChange( value, changes ); |
There was a problem hiding this comment.
@joshuatf note that I did end up updating this logic and exposing the changes as a second param.
I realized we did need this to be triggered all the time, especially in a async multi select, where a user uses there keyboard to select an item. The input value gets cleared afterwards, so if the user hits a down arrow after, the results of the original search value are still being shown, which is wrong.
mattsherman
left a comment
mattsherman
left a comment
There was a problem hiding this comment.
Overall, looks like a great start on this!
I hit some confusing behavior in Storybook. And, I think there are some issues with the debounce handling.
Bigger picture, I'm finding it a little confusing how the consumer handles "client side" filtering vs fetching... I feel like things could be made more clear and explicit for the consumer, instead of depending on having total and pageSize set appropriately. In fact, I naively feel like the consumer should just be able to set onSearch (or, getFilteredItems... see below) and it can handle whether it is doing things "client side" or going off to the network.
Even bigger picture... I wonder if the separate async wrapper is really needed. Why not just bake this into the base select control and have items and getFilteredItems support passing in a Promise? Or, maybe even just have getFilteredItems... do we really need both items and getFilteredItems? Does the select control need to ever know if it is dealing with "all" the items, or does it really just need to show and allow selection of the filtered items?
| }; | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
Why the Fragment wrapper?
This is unnecessary and makes the story less useful than it could be otherwise. Ideally, stories should be set up to support Storybook controls (https://storybook.js.org/docs/react/essentials/controls).
If you need a wrapper, you can use Storybook decorators.
See the DateTimePickerControl stories for an example of how to structure things to handle controls and decorators.
There was a problem hiding this comment.
It would be nice on this story to use a decorator to show the sample items that will be fetched. That would make it easier to use the story.
| .toLowerCase() | ||
| .includes( value?.toLowerCase() || '' ) | ||
| ) | ||
| .sort( () => 0.5 - Math.random() ); |
There was a problem hiding this comment.
Why the random sort order? It makes it harder to understand what is happening in the story and harder to understand if things are working correctly.
| const [ isFetching, setIsFetching ] = useState( false ); | ||
| }; | ||
|
|
||
| export const AsyncWithClientSideFiltering: React.FC = () => { |
There was a problem hiding this comment.
I don't really know what's going on here... whether it is a bug in the story, or the component, but I'm getting really confusing behavior when I type things in...
Type r, and only Pear, and Grape appear (why not Orange too?). Continue typing, so ra, and now only Orange appears (why no Grape)?!
There was a problem hiding this comment.
I don't understand where the client side filtering comes into play here. I keep on looking at this story and the only the setting of the pageSize and total props differ.
Ah...
If a user decides to pass in the pageSize and total, if the total is less then the pageSize it will do the filtering on the client side and not hit onSearch for every key stroke.
Okay. I think I get it.
Would be nice to use Storybook actions (see DataTimePickerControl example mentioned earlier) to log when onSearch is called. That would help to illustrate what is going on here.
| onInputChange?: ( value: string | undefined ) => void; | ||
| onInputChange?: ( | ||
| value: string | undefined, | ||
| changes: Partial< Omit< UseComboboxState< ItemType >, 'inputValue' > > |
There was a problem hiding this comment.
It is unclear to me what changes means here... is it all of the combobox state. Why does a consumer care about this?
There was a problem hiding this comment.
In fact, bigger picture, why does the consumer need onInputChange at all? Shouldn't this implementation be hidden from the consumer?
| SelectControlProps< ItemType >, | ||
| 'onInputChange' | 'items' | 'children' | ||
| > & { | ||
| onSearch: ( value: string | undefined ) => Promise< ItemType[] >; |
There was a problem hiding this comment.
Why | undefined? Why would we ever call onSearch with value === undefined?
| items?: ItemType[]; | ||
| triggerInitialFetch?: boolean; | ||
| pageSize?: number; | ||
| total?: number; |
There was a problem hiding this comment.
A slightly more descriptive prop name would be nice. What is this?
| return ( | ||
| <Menu isOpen={ isOpen } getMenuProps={ getMenuProps }> | ||
| { isFetching ? ( | ||
| <Spinner /> |
There was a problem hiding this comment.
Spinner feels a bit odd in a dropdown like this. Maybe if it had some text with it as well it would be better.
| } | ||
| }, [ remainingSelectControlProps.disabled ] ); | ||
|
|
||
| const searchDebounced = useMemo( |
There was a problem hiding this comment.
Might want to consider useDebounce (see #34963)
There was a problem hiding this comment.
Otherwise, I think that this will suffer similar issues to what I found in DateTimePickerControl. But, since you use debounce instead of useDebounce, this might actually be "more" okay, as your debounce won't get canceled.
| { ...remainingSelectControlProps } | ||
| getFilteredItems={ getFilteredItems } | ||
| items={ fetchedItems } | ||
| onInputChange={ onInputChange } |
There was a problem hiding this comment.
Why isn't getFilteredItems enough for the consumer? Why is onInputChange needed as well?
|
@louwie17 It would also be useful to add a |
|
@louwie17 it would be great to have some unit tests as well for this component. |
|
@mattsherman thanks for the detailed review on this, also much appreciate your view and input. I like your suggestion of having |
|
@louwie17 Sounds good. If we don't need this control now, I'd definitely be supportive of putting it on the back-burner for now. |
Yeah in that case I will close this for now and create a separate issue for this. As there is some general support that could be added to the select control. |
|
Closing this in favour of a follow up as part of this issue: #35076 |
All Submissions:
Changes proposed in this Pull Request:
This adds an
AsyncSelectControlcomponent that is a wrapper around theSelectControlcomponent.It adds a
onSearchprop that expects a promise with the search results.If a user decides to pass in the
pageSizeandtotal, if thetotalis less then thepageSizeit will do the filtering on the client side and not hitonSearchfor every key stroke.Partially addresses #34331
How to test the changes in this Pull Request:
pnpm --filter=@woocommerce/storybook storybookAsyncandAsync with client side filteringunder the experimental select control.Other information:
pnpm --filter=<project> run changelog add?FOR PR REVIEWER ONLY: