From da80b09e7efa8f6ab3a93fb504b1ec5ec923561e Mon Sep 17 00:00:00 2001 From: Marc Thomas Date: Tue, 9 Jan 2024 11:23:00 +0000 Subject: [PATCH 1/4] chore: fix vanishing filters bug --- .../components/Filters/Filters.stories.tsx | 201 ++++++++++++++++++ .../components/FiltersBar/FiltersBar.tsx | 4 +- .../FiltersBar/tests/FiltersBar.test.tsx | 26 +++ 3 files changed, 230 insertions(+), 1 deletion(-) diff --git a/polaris-react/src/components/Filters/Filters.stories.tsx b/polaris-react/src/components/Filters/Filters.stories.tsx index dfefa8900db..338eb97c34a 100644 --- a/polaris-react/src/components/Filters/Filters.stories.tsx +++ b/polaris-react/src/components/Filters/Filters.stories.tsx @@ -1516,3 +1516,204 @@ export function WithFilterBarHidden() { ); } + +export function WithAllFiltersPinned() { + const [accountStatus, setAccountStatus] = useState(null); + const [moneySpent, setMoneySpent] = useState(null); + const [taggedWith, setTaggedWith] = useState(''); + const [queryValue, setQueryValue] = useState(''); + + const handleAccountStatusChange = useCallback( + (value) => setAccountStatus(value), + [], + ); + const handleMoneySpentChange = useCallback( + (value) => setMoneySpent(value), + [], + ); + const handleTaggedWithChange = useCallback( + (value) => setTaggedWith(value), + [], + ); + const handleFiltersQueryChange = useCallback( + (value) => setQueryValue(value), + [], + ); + const handleAccountStatusRemove = useCallback( + () => setAccountStatus(null), + [], + ); + const handleMoneySpentRemove = useCallback(() => setMoneySpent(null), []); + const handleTaggedWithRemove = useCallback(() => setTaggedWith(''), []); + const handleQueryValueRemove = useCallback(() => setQueryValue(''), []); + const handleFiltersClearAll = useCallback(() => { + handleAccountStatusRemove(); + handleMoneySpentRemove(); + handleTaggedWithRemove(); + handleQueryValueRemove(); + }, [ + handleAccountStatusRemove, + handleMoneySpentRemove, + handleQueryValueRemove, + handleTaggedWithRemove, + ]); + + const filters = [ + { + key: 'accountStatus', + label: 'Account status', + filter: ( + + ), + shortcut: true, + pinned: true, + }, + { + key: 'taggedWith', + label: 'Tagged with', + filter: ( + + ), + shortcut: true, + pinned: true, + }, + { + key: 'moneySpent', + label: 'Money spent', + filter: ( + + ), + shortcut: true, + pinned: true, + }, + ]; + + const appliedFilters: FiltersProps['appliedFilters'] = []; + if (!isEmpty(accountStatus)) { + const key = 'accountStatus'; + appliedFilters.push({ + key, + label: disambiguateLabel(key, accountStatus), + onRemove: handleAccountStatusRemove, + }); + } + if (!isEmpty(moneySpent)) { + const key = 'moneySpent'; + appliedFilters.push({ + key, + label: disambiguateLabel(key, moneySpent), + onRemove: handleMoneySpentRemove, + }); + } + if (!isEmpty(taggedWith)) { + const key = 'taggedWith'; + appliedFilters.push({ + key, + label: disambiguateLabel(key, taggedWith), + onRemove: handleTaggedWithRemove, + }); + } + + return ( +
+ + + } + flushFilters + items={[ + { + id: '341', + url: '#', + name: 'Mae Jemison', + location: 'Decatur, USA', + }, + { + id: '256', + url: '#', + name: 'Ellen Ochoa', + location: 'Los Angeles, USA', + }, + ]} + renderItem={(item) => { + const {id, url, name, location} = item; + const media = ; + + return ( + + + {name} + +
{location}
+ + ); + }} + /> + +
+ ); + + function disambiguateLabel(key, value) { + switch (key) { + case 'moneySpent': + return `Money spent is between $${value[0]} and $${value[1]}`; + case 'taggedWith': + return `Tagged with ${value}`; + case 'accountStatus': + return value.map((val) => `Customer ${val}`).join(', '); + default: + return value; + } + } + + function isEmpty(value) { + if (Array.isArray(value)) { + return value.length === 0; + } else { + return value === '' || value == null; + } + } +} diff --git a/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx b/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx index 019b6eb7299..2e6c66ca5de 100644 --- a/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx +++ b/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx @@ -185,7 +185,9 @@ export function FiltersBar({ setLocalPinnedFilters([]); onClearAll?.(); }; - const shouldShowAddButton = filters.some((filter) => !filter.pinned); + const shouldShowAddButton = + filters.some((filter) => !filter.pinned) || + filters.length !== localPinnedFilters.length; const pinnedFiltersMarkup = pinnedFilters.map( ({key: filterKey, ...pinnedFilter}) => { diff --git a/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx b/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx index 403f831902e..622780a5a84 100644 --- a/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx +++ b/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx @@ -6,6 +6,7 @@ import {ActionList} from '../../../../ActionList'; import {FiltersBar} from '../FiltersBar'; import type {FiltersBarProps} from '../FiltersBar'; import {FilterPill} from '../../FilterPill'; +import {Button} from '../../../../Button'; describe('', () => { let originalScroll: any; @@ -388,4 +389,29 @@ describe('', () => { ], }); }); + + it('will show the add filter button when clearing all pinned filters', () => { + const scrollSpy = jest.fn(); + HTMLElement.prototype.scroll = scrollSpy; + const filters = defaultProps.filters.map((filter) => ({ + ...filter, + pinned: true, + })); + + const wrapper = mountWithApp( + , + ); + + const clearAll = wrapper.find(Button, { + children: 'Clear all', + }); + + wrapper.act(() => { + clearAll?.trigger('onClick'); + }); + + expect(wrapper).toContainReactComponent('div', { + className: 'AddFilterActivator', + }); + }); }); From f8bf83151d05179cf6c2bf98002b3aa8a1884bfe Mon Sep 17 00:00:00 2001 From: Marc Thomas Date: Tue, 9 Jan 2024 11:23:32 +0000 Subject: [PATCH 2/4] chore: changeset --- .changeset/serious-vans-sin.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/serious-vans-sin.md diff --git a/.changeset/serious-vans-sin.md b/.changeset/serious-vans-sin.md new file mode 100644 index 00000000000..9ba84efa91b --- /dev/null +++ b/.changeset/serious-vans-sin.md @@ -0,0 +1,5 @@ +--- +'@shopify/polaris': minor +--- + +[FiltersBar] Fixed bug where filters would disappear from the FiltersBar when clicking the Clear all button From 43c02b5b51ecf43c2fbf98f1771128f04d21e210 Mon Sep 17 00:00:00 2001 From: Marc Thomas Date: Wed, 10 Jan 2024 08:09:25 +0000 Subject: [PATCH 3/4] chore: remove clear all until applied filters exist --- .../components/FiltersBar/FiltersBar.tsx | 37 +++++++++---------- .../FiltersBar/tests/FiltersBar.test.tsx | 15 +++++++- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx b/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx index 2e6c66ca5de..083ece0fd51 100644 --- a/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx +++ b/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx @@ -238,26 +238,25 @@ export function FiltersBar({ ) : null; - const clearAllMarkup = - appliedFilters?.length || localPinnedFilters.length ? ( -
+ -
- ) : null; + {i18n.translate('Polaris.Filters.clearFilters')} + + + ) : null; return (
', () => { ...filter, pinned: true, })); - + const appliedFilters = [ + { + ...defaultProps.filters[2], + label: 'Bux', + value: ['Bux'], + onRemove: jest.fn(), + }, + ]; const wrapper = mountWithApp( - , + , ); const clearAll = wrapper.find(Button, { From 6f8b114793cb1314af4fe332e31ec4a3cfc11de1 Mon Sep 17 00:00:00 2001 From: Marc Thomas Date: Fri, 12 Jan 2024 09:35:10 +0000 Subject: [PATCH 4/4] chore: ensure pinned filters from props persist --- .../components/FiltersBar/FiltersBar.tsx | 12 +++- .../FiltersBar/tests/FiltersBar.test.tsx | 62 ++++++++++++------- 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx b/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx index 083ece0fd51..489c14a9278 100644 --- a/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx +++ b/polaris-react/src/components/Filters/components/FiltersBar/FiltersBar.tsx @@ -78,6 +78,10 @@ export function FiltersBar({ }; const appliedFilterKeys = appliedFilters?.map(({key}) => key); + const pinnedFromPropsKeys = filters + .filter(({pinned}) => pinned) + .map(({key}) => key); + const pinnedFiltersFromPropsAndAppliedFilters = filters.filter( ({pinned, key}) => { const isPinnedOrApplied = @@ -182,7 +186,7 @@ export function FiltersBar({ ); const handleClearAllFilters = () => { - setLocalPinnedFilters([]); + setLocalPinnedFilters(pinnedFromPropsKeys); onClearAll?.(); }; const shouldShowAddButton = @@ -194,7 +198,11 @@ export function FiltersBar({ const appliedFilter = appliedFilters?.find(({key}) => key === filterKey); const handleFilterPillRemove = () => { setLocalPinnedFilters((currentLocalPinnedFilters) => - currentLocalPinnedFilters.filter((key) => key !== filterKey), + currentLocalPinnedFilters.filter((key) => { + const isMatchedFilters = key === filterKey; + const isPinnedFilterFromProps = pinnedFromPropsKeys.includes(key); + return !isMatchedFilters || isPinnedFilterFromProps; + }), ); appliedFilter?.onRemove(filterKey); }; diff --git a/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx b/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx index 0fdb0df11d3..b2b4f2229af 100644 --- a/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx +++ b/polaris-react/src/components/Filters/components/FiltersBar/tests/FiltersBar.test.tsx @@ -390,39 +390,57 @@ describe('', () => { }); }); - it('will show the add filter button when clearing all pinned filters', () => { + it('will keep a pinned filter from props pinned when clearing', () => { + const appliedFilters = [ + { + ...defaultProps.filters[1], + label: 'Bar 2', + value: ['Bar 2'], + onRemove: jest.fn(), + }, + ]; const scrollSpy = jest.fn(); HTMLElement.prototype.scroll = scrollSpy; - const filters = defaultProps.filters.map((filter) => ({ - ...filter, - pinned: true, - })); + const wrapper = mountWithApp( + , + ); + + wrapper + .find(FilterPill, { + label: 'Bar 2', + })! + .trigger('onRemove'); + + expect(wrapper).toContainReactComponentTimes(FilterPill, 1); + }); + + it('will keep a pinned filter from props pinned when clearing all', () => { const appliedFilters = [ + { + ...defaultProps.filters[0], + label: 'Bar 2', + value: ['Bar 2'], + onRemove: jest.fn(), + }, { ...defaultProps.filters[2], - label: 'Bux', - value: ['Bux'], + label: 'Bar 2', + value: ['Bar 2'], onRemove: jest.fn(), }, ]; + const scrollSpy = jest.fn(); + HTMLElement.prototype.scroll = scrollSpy; const wrapper = mountWithApp( - , + , ); - const clearAll = wrapper.find(Button, { - children: 'Clear all', - }); - - wrapper.act(() => { - clearAll?.trigger('onClick'); - }); + wrapper + .find(Button, { + children: 'Clear all', + })! + .trigger('onClick'); - expect(wrapper).toContainReactComponent('div', { - className: 'AddFilterActivator', - }); + expect(wrapper).toContainReactComponentTimes(FilterPill, 1); }); });