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

Forward autocompleter prop from CompareFilter to Search #6911

Merged
merged 4 commits into from
May 13, 2021

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Apr 29, 2021

(Follow-up to #6880, to make `ReportFilters support custom Search)

  • Forward autocompleter prop from CompareFilter to Search.
    Allow, to use the custom type of search, previously it was complaining about lack of autocompleter, even though it was provided.

  • Move path in Storybooks example to a parameter, to allow setting it in unit tests.

  • Add few tests for FilterPicker.

    • it renders the basic storybook example without throwing an error
    • it forwards autocompleter & type props

Fixes: #6890
Needed for:242-gh-woocommerce/google-listings-and-ads

Accessibility

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

No new texts or animations were introduced, existing ones behave as before.

Screenshots

Detailed test instructions:

  • Create CompareFilter with type="custom" and autocompleter
    <CompareFilter
    	type: 'custom',
    	autocompleter: {...} // Even the one copied from exiting ones.
    />
  • Check for errors.

Doubts:

  1. I was wondering whether maybe the more sustainable and flexible solution would be to either forward all the props to allow customization of the Search element, and do not curate the list of forwarded ones.

tomalec added 2 commits April 29, 2021 17:29
Test that it renders the basic storybook example without throwing an error.

Move `path` in Storybook's example to a parameter, to allow setting it in unit tests.
Allow, to use the `custom` type of search, previously it was complaining about lack of `autocompleter`, even though it was provided.
Fixes: #6890
@@ -91,7 +91,7 @@ export class CompareFilter extends Component {
}

render() {
const { labels, type } = this.props;
const { labels, type, autocompleter } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about changing this to searchAutocompleter? That could make it more obvious that it is used by search.

Copy link
Member Author

@tomalec tomalec May 10, 2021

Choose a reason for hiding this comment

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

But then it would be inconsistent with type which is also kind of searchType:

	/**
	 * Which type of autocompleter should be used in the Search
	 */
	type: PropTypes.string.isRequired,
	/**
	 * The custom autocompleter to be forwarded to the `Search` component.
	 */
	autocompleter: PropTypes.object,

and changing type would be a breaking change.

As mentioned above I was thinking of doing it another way around and provide all props together like:

	searchProps: PropTypes.shape( {
		placeholder: PropTypes.string,
		type: PropTypes.string.isRequired,
		autocompleter: PropTypes.object,
	} ),

Or even accepting the entire component as a prop.

Personally, to keep the API and prop names consistent, I'd either:

  1. Leave it as is, or
  2. Group all search props together.

WDYT?

Copy link
Contributor

@becdetat becdetat left a comment

Choose a reason for hiding this comment

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

Apart from my one comment, this works well and the code is good. Pre-approving. Thanks!

@tomalec
Copy link
Member Author

tomalec commented May 13, 2021

I'm merging it as is. autocompleter vs. searchAutocompleter discussion could be continued at woocommerce/woocommerce#32254

@tomalec tomalec merged commit a9ad7f7 into main May 13, 2021
@tomalec tomalec deleted the fix/6890-comparefilter-autocompleter branch May 13, 2021 15:20
ObliviousHarmony pushed a commit to woocommerce/woocommerce that referenced this pull request Mar 18, 2022
…merce/woocommerce-admin#6911)

- Forward `autocompleter` prop from `CompareFilter` to `Search`.
	Allow, to use the `custom` type of search, 
	previously it was complaining about lack of `autocompleter`,
	even though it was provided.

- Move `path` in Storybooks example to a parameter, to allow setting it in unit tests.
- Add few tests for FilterPicker. 
	- it renders the basic storybook example without throwing an error
	- it forwards `autocompleter` & `type` props

Fixes: woocommerce/woocommerce-admin#6890
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CompareFilter does not forward autocompleter setting to the Search
4 participants