Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Make Search accept sync autocompleter.options. #6884

Merged
merged 10 commits into from
May 3, 2021
Merged

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Apr 26, 2021

Make Search component accept autocompleter.options that meet the requirements stated in the docs:

May be an array, a function that returns an array, or a function that returns a promise for an array.

Fixes #6061.

Accessibility

  • I've tested using only a keyboard (no mouse)

No texts or animations were changed

Screenshots

Detailed test instructions:

  • Create Search component with autocompleter prop, that contains options property with a static array, or function that returns an array:
const customAutocompleter = {
	...productsAutocompleter,
	options: [
		{ name: 'Apple', id: 1 },
		{ name: 'Orange', id: 2 },
		{ name: 'Grapes', id: 3 },
	],
};
render(
	<Search
		type="custom"
		autocompleter={ customAutocompleter }
	/>
);

See the attached tests.

Additional notes:

  1. Jest shows a React warning, that's a result of an issue that was not introduced here: React warns about memory leak in Search/SelectControl component. #6883
  2. I also considered using async syntax to make fetchOptions look cleaner, but decided to use the same structure that is used in wordpress/components.Autocompleter https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/autocomplete/index.js#L168-L171
    Alternative, async/await version is waiting at https://github.com/woocommerce/woocommerce-admin/compare/fix/6061-search-options-async

Doubts:

  1. I think the delay of arbitrary 1000ms makes the test less stable, but I didn't find any official/documented way to wait for the exact promise/event so I can check for options to be rendered.
    With plain HTML (custom) elements, I'd expect either filterPicker to dispatch a DOM event or expose a promise, or at least DOM itself would notify MutationObserver with option/<search-element> being added, but I'm not sure how it's meant to be monitored here.

to let CI wait longer for options to be rendered.
Copy link
Contributor

@jeffstieler jeffstieler left a comment

Choose a reason for hiding this comment

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

Pre-approving, but asking for some small tweaks to the test.

I had to modify the config slightly to test - our autocompleters do deviate from Gutenberg in more ways like:

// This is slightly different than gutenberg/Autocomplete, we don't support different methods
// of replace/insertion, so we can just return the value.
getOptionCompletion( attribute ) {
const value = {
key: attribute.id,
label: attribute.name,
};
return value;
},

The shape of the options are:

const nameOption = {
key: 'name',
label,
value: { id: query, name: query },
};

readme.txt Outdated
@@ -314,12 +315,12 @@ Release and roadmap notes are available on the [WooCommerce Developers Blog](htt
- Dev: Revert work done in #4857 for automated shipping after OBW is completed #5971
- Add: Welcome modal when coming from Calypso #6004
- Enhancement: Add an a/b experiment for installing free business features #5786
- Dev: Add `onChangeCallback` feature to the wc-admin <Form> component #5786
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we revert all these newline changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Phew... resolving newline conflicts from revert after your merge main, was the hardest part of this PR ;)

// Emulate typing to render available options.
userEvent.type( getByRole( 'combobox' ), 'A' );
// Wait for async options procesing.
await delay( 1000 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than use a delay, you can use waitFor:

userEvent.type( getByRole( 'combobox' ), query );
// Wait for the search promise to resolve.
await waitFor( () =>
expect(
getByRole( 'option', { name: 'lorem 1' } )
).toBeInTheDocument()
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, TIL.

@jeffstieler jeffstieler added has tests needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Apr 27, 2021
@tomalec
Copy link
Member Author

tomalec commented Apr 29, 2021

Pre-approving, but asking for some small tweaks to the test.

I had to modify the config slightly to test - our autocompleters do deviate from Gutenberg in more ways like:

// This is slightly different than gutenberg/Autocomplete, we don't support different methods
// of replace/insertion, so we can just return the value.
getOptionCompletion( attribute ) {
const value = {
key: attribute.id,
label: attribute.name,
};
return value;
},

The shape of the options are:

const nameOption = {
key: 'name',
label,
value: { id: query, name: query },
};

@jeffstieler,
If you have a working example of the smallest FilterPicker-specific custom .autocompleter that works, I'd be thankful if you post it, as I'm struggling to create one for a couple of days now.

Also, do you mean by this comment, I should change something in this test's options?
I use the products autocomplete here, which - as far as I can tell - uses the format of options I provide: {id, name}

@jeffstieler
Copy link
Contributor

@tomalec here's a sample I used for testing (working off the fruit example from Gutenberg):

{
			label: __( 'Fruit', 'woocommerce-admin' ),
			value: 'select_fruit',
			chartMode: 'item-comparison',
			subFilters: [
				{
					component: 'Search',
					value: 'single_fruit',
					chartMode: 'item-comparison',
					path: [ 'select_fruit' ],
					settings: {
						type: 'custom',
						param: 'fruit',
						getLabels: ( fruitId ) => {
							return Promise.resolve(
								fruits
									.filter( ( { id } ) => id == fruitId )
									.map( ( { id, name } ) => ( {
										key: id,
										label: name,
									} ) )
							);
						},
						labels: {
							placeholder: __(
								'Type to search for a fruit',
								'woocommerce-admin'
							),
							button: __( 'Fruit', 'woocommerce-admin' ),
						},
						autocompleter: {
							// The option data
							options: fruits,
							getOptionIdentifier: ( fruit ) => {
								return fruit.id;
							},
							// Returns a label for an option like "🍊 Orange"
							getOptionLabel: ( option ) => (
								<span>
									<span className="icon">
										{ option.visual }
									</span>
									{ option.name }
								</span>
							),
							// Declares that options should be matched by their name
							getOptionKeywords: ( option ) => [ option.name ],
							// This is slightly different than gutenberg/Autocomplete, we don't support different methods
							// of replace/insertion, so we can just return the value.
							getOptionCompletion( attribute ) {
								const value = {
									key: attribute.id,
									label: attribute.name,
								};
								return value;
							},
						},
					},
				},
			],
		},

You can drop that into a report config (client/analytics/report/*/config.js) to test.

@tomalec
Copy link
Member Author

tomalec commented Apr 30, 2021

Thanks for the example. I added a minimalistic autocompleter config to the test file explicitly.
The PR is ready for another round of review.

@tomalec tomalec merged commit d30d0b2 into main May 3, 2021
@tomalec tomalec deleted the fix/6061-search-options branch May 3, 2021 17:18
ObliviousHarmony pushed a commit to woocommerce/woocommerce that referenced this pull request Mar 18, 2022
…mmerce-admin#6884)

Co-authored-by: Jeff Stieler <[email protected]>

Make `Search` component accept `autocompleter.options` that meet the requirements stated in [the docs](https://github.com/WordPress/gutenberg/tree/trunk/packages/components/src/autocomplete#options): 
> May be an array, a function that returns an array, or a function that returns a promise for an array.


Fixes woocommerce/woocommerce-admin#6061.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. @woocommerce/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search component does not accept static options Array in autocomplete
2 participants