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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Unreleased

- Fix `autocompleter` for custom Search in `CompareFilter` #6911
- SelectControl: automatically scroll to selected options when list is displayed. #6906
- SelectControl: no longer auto selects on rendering list. #6906
- Make `Search` accept synchronous `autocompleter.options`. #6884
Expand Down
7 changes: 6 additions & 1 deletion packages/components/src/compare-filter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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?

const { selected } = this.state;
return (
<Card className="woocommerce-filters__compare">
Expand All @@ -100,6 +100,7 @@ export class CompareFilter extends Component {
</CardHeader>
<CardBody>
<Search
autocompleter={ autocompleter }
type={ type }
selected={ selected }
placeholder={ labels.placeholder }
Expand Down Expand Up @@ -165,6 +166,10 @@ CompareFilter.propTypes = {
* 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,
};

CompareFilter.defaultProps = {
Expand Down
7 changes: 3 additions & 4 deletions packages/components/src/compare-filter/stories/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { CompareFilter } from '@woocommerce/components';

const path = new URL( document.location ).searchParams.get( 'path' );
const query = {};
const compareFilter = {
type: 'products',
Expand All @@ -19,9 +18,9 @@ const compareFilter = {
},
};

export const Basic = () => (
<CompareFilter path={ path } query={ query } { ...compareFilter } />
);
export const Basic = ( {
path = new URL( document.location ).searchParams.get( 'path' ),
} ) => <CompareFilter path={ path } query={ query } { ...compareFilter } />;

export default {
title: 'WooCommerce Admin/components/CompareFilter',
Expand Down
70 changes: 70 additions & 0 deletions packages/components/src/compare-filter/test/compare-filter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';

/**
* Internal dependencies
*/
import { Basic } from '../stories/index';
import { CompareFilter } from '../index';
import Search from '../../search';
import productAutocompleter from '../../search/autocompleters/product';
// Due to Jest implementation we cannot mock it only for specific tests.
// If your test requires non-mocked Search, move them to another test file.
jest.mock( '../../search' );
Search.mockName( 'Search' );

describe( 'CompareFilter', () => {
let props;
beforeEach( () => {
props = {
path: '/foo/bar',
type: 'products',
param: 'product',
getLabels() {
return Promise.resolve( [] );
},
labels: {
helpText: 'Select at least two to compare',
placeholder: 'Search for things to compare',
title: 'Compare Things',
update: 'Compare',
},
};
} );
it( 'should render the example from the storybook', () => {
const path = '/story/woocommerce-admin-components-comparefilter--basic';

expect( function () {
render( <Basic path={ path } /> );
} ).not.toThrow();
} );

it( 'should forward the `type` prop the Search component', () => {
props.type = 'custom';

render( <CompareFilter { ...props } /> );

// Check that Search component received the prop, without checking its behavior/internals/implementation details.
expect( Search ).toHaveBeenLastCalledWith(
expect.objectContaining( {
type: 'custom',
} ),
expect.anything()
);
} );
it( 'should forward the `autocompleter` prop the Search component', () => {
props.autocompleter = productAutocompleter;

render( <CompareFilter { ...props } /> );

// Check that Search component received the prop, without checking its behavior/internals/implementation details.
expect( Search ).toHaveBeenLastCalledWith(
expect.objectContaining( {
autocompleter: productAutocompleter,
} ),
expect.anything()
);
} );
} );
2 changes: 2 additions & 0 deletions readme.txt
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ Release and roadmap notes are available on the [WooCommerce Developers Blog](htt
== Changelog ==

== Unreleased ==

- Fix: Autocompleter for custom Search in CompareFilter #6911
- Enhancement: Adding Slotfills for remote payments and SettingsForm component. #6932
- Fix: Make `Search` accept synchronous `autocompleter.options`. #6884
- Add: Consume remote payment methods on frontend #6867
Expand Down