-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Filters] Fix bug where changes to appliedFilters
prop don't get respected
#10566
Conversation
/snapit |
🫰✨ Thanks @mrcthms! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230919152921 yarn add @shopify/polaris@0.0.0-snapshot-release-20230919152921 yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230919152921 yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230919152921 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch 😅 Thanks @mrcthms!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching and fixing this, @mrcthms!
Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@11.20.0 ### Minor Changes - [#10477](#10477) [`790a001cd`](790a001) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated semantic `color` tokens - [#10478](#10478) [`8be227e0c`](8be227e) Thanks [@MaxCloutier](https://github.com/MaxCloutier)! - Added `allowFiltering` prop on `ActionList`, and `filterActions` prop on Page Header - [#9445](#9445) [`7be9c243a`](7be9c24) Thanks [@m4thieulavoie](https://github.com/m4thieulavoie)! - Added support for subheaders and selection of a range of `IndexTable.Rows` -- See the [With subheaders](https://polaris.shopify.com/components/tables/index-table) example on polaris.shopify.com for how to properly configure - `IndexTable.Row` - Added support for setting the `indeterminate` value on the `selected` prop - Added the `selectionRange` prop to specify a range of other consecutive, related rows selected when the row is selected - Added the `rowType` prop to indicate the relationship or role of the row's contents (defaults to `data`, `subheader` renders the row to look and behave like the table header row) Added support for setting accessibility attributes on `IndexTable.Cell` - `IndexTable.Cell` - Added the `as` prop to support rendering the cell as a `th` element if it is serving as a subheading cell - Added support for the `headers` attribute to manually associate all headers when the cell is described by more than its column heading - Added support for the `colSpan` attribute to specify the number of the columns that the cell element should extend to - Added support for the `scope` attribute to indicate whether the `th` is a header for a column, row, or group of columns or rows - [#10490](#10490) [`863f15ff2`](863f15f) Thanks [@mrcthms](https://github.com/mrcthms)! - Add new `IndexFiltersManager` for allowing disabling of Page Header actions when in Filtering or EditingColumns mode - [#10566](#10566) [`9fed74317`](9fed743) Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed a bug in `Filters` where changes to the `appliedFilters` prop were not being handled ### Patch Changes - [#10404](#10404) [`5acfcec04`](5acfcec) Thanks [@jesstelford](https://github.com/jesstelford)! - Scoped CSS variables for non-responsive props on `Tooltip`, `RangeSlider`, `ProgressBar`, and `HorizontalStack`. - [#10582](#10582) [`3efbc1b4e`](3efbc1b) Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed the focus states of actions within the Page Header component - [#10492](#10492) [`d5ff72dec`](d5ff72d) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated Storybook stories to be wrapped with an empty FrameContext.Provider - Updated dependencies \[[`fe1aac1b5`](fe1aac1), [`790a001cd`](790a001), [`63cf3ad24`](63cf3ad), [`120e96eae`](120e96e)]: - @shopify/polaris-tokens@7.10.0 ## @shopify/polaris-tokens@7.10.0 ### Minor Changes - [#10465](#10465) [`fe1aac1b5`](fe1aac1) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated private primitive `colors` - [#10477](#10477) [`790a001cd`](790a001) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated semantic `color` tokens - [#10600](#10600) [`63cf3ad24`](63cf3ad) Thanks [@lgriffee](https://github.com/lgriffee)! - Added public primitive and semantic `shadow` token scales ### Patch Changes - [#10485](#10485) [`120e96eae`](120e96e) Thanks [@lgriffee](https://github.com/lgriffee)! - Updated public primitive `base` and `light-uplift` theme scales ## @shopify/polaris-migrator@0.22.5 ### Patch Changes - [#10575](#10575) [`ea6b54284`](ea6b542) Thanks [@aveline](https://github.com/aveline)! - Handled `buttonFrom` and `buttonsFrom` functions in `Button` migration - Updated dependencies \[[`fe1aac1b5`](fe1aac1), [`790a001cd`](790a001), [`63cf3ad24`](63cf3ad), [`120e96eae`](120e96e)]: - @shopify/polaris-tokens@7.10.0 - @shopify/stylelint-polaris@14.0.5 ## @shopify/stylelint-polaris@14.0.5 ### Patch Changes - Updated dependencies \[[`fe1aac1b5`](fe1aac1), [`790a001cd`](790a001), [`63cf3ad24`](63cf3ad), [`120e96eae`](120e96e)]: - @shopify/polaris-tokens@7.10.0 ## polaris.shopify.com@0.57.8 ### Patch Changes - [#10605](#10605) [`9748b0838`](9748b08) Thanks [@laurkim](https://github.com/laurkim)! - Updated logic for rendering `color` custom property previews in `TokenList` - [#10573](#10573) [`da09e0b8c`](da09e0b) Thanks [@kyledurand](https://github.com/kyledurand)! - Updated copy url to change browser url - Updated dependencies \[[`fe1aac1b5`](fe1aac1), [`5acfcec04`](5acfcec), [`790a001cd`](790a001), [`8be227e0c`](8be227e), [`63cf3ad24`](63cf3ad), [`7be9c243a`](7be9c24), [`863f15ff2`](863f15f), [`3efbc1b4e`](3efbc1b), [`d5ff72dec`](d5ff72d), [`120e96eae`](120e96e), [`9fed74317`](9fed743)]: - @shopify/polaris-tokens@7.10.0 - @shopify/polaris@11.20.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Currently `appliedFilters` can be `undefined`, except when it is when we try to `setLocalPinnedFilters`, we'll do so forever because we fail the check to decide whether to try set state or not every time, as `!allAppliedFilterKeysInLocalPinnedFilters` will always be `true`. To avoid this, we should only try to make the local pinned filters match if we actually have any applied filters - and it's not `undefined`. Found this while trying to bump to `v11.20.0`. This was introduced in: * #10566
Currently `appliedFilters` can be `undefined`, except when it is when we try to `setLocalPinnedFilters`, we'll do so forever because we fail the check to decide whether to try set state or not every time, as `!allAppliedFilterKeysInLocalPinnedFilters` will always be `true`. To avoid this, we should only try to make the local pinned filters match if we actually have any applied filters - and it's not `undefined`. Found this while trying to bump to `v11.20.0`. This was introduced in: * #10566
Currently `appliedFilters` can be `undefined`, except when it is when we try to `setLocalPinnedFilters`, we'll do so forever because we fail the check to decide whether to try set state or not every time, as `!allAppliedFilterKeysInLocalPinnedFilters` will always be `true`. To avoid this, we should only try to make the local pinned filters match if we actually have any applied filters - and it's not `undefined`. Found this while trying to bump to `v11.20.0`. This was introduced in: * #10566
Currently `appliedFilters` can be `undefined`, except when it is when we try to `setLocalPinnedFilters`, we'll do so forever because we fail the check to decide whether to try set state or not every time, as `!allAppliedFilterKeysInLocalPinnedFilters` will always be `true`. To avoid this, we should only try to make the local pinned filters match if we actually have any applied filters - and it's not `undefined`. Found this while trying to bump to `v11.20.0`. This was introduced in: * #10566
…#10711) ### WHY are these changes introduced? Currently `appliedFilters` can be `undefined`, except when it is when we try to `setLocalPinnedFilters`, we'll do so forever because we fail the check to decide whether to try set state or not every time, as `!allAppliedFilterKeysInLocalPinnedFilters` will always be `true`. To avoid this, we should only try to make the local pinned filters match if we actually have any applied filters - and it's not `undefined`. Found this while trying to bump to `v11.20.0` from `v11.19.0`. This was introduced in: * #10566 ### WHAT is this pull request doing? Checking `appliedFilters` is `defined` before trying to `setLocalPinnedFilters`. ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) Example test that shows a bunch of re-renders ```tsx import React from 'react'; import {render} from '@testing-library/react'; import {Filters, PolarisTestProvider} from '@shopify/polaris'; function Test() { return ( <PolarisTestProvider> <Filters filters={[]} /> </PolarisTestProvider> ); } describe('Testing', () => { it('does thing', () => { render(<Test />); }); }); ``` <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? This PR takes another approach to the applied filters and pinned filters logic. In [this PR](#10566), we tried to solve for an issue on a specific index table, the Orders index, which loads the filter data asynchronously. However, I now feel this approach is wrong and we should be solving for this problem in another way. Due to how the Filters component was built before, all the logic regarding state management of pinned and applied filters was generated when the Filters component mounted, yet if there were no filters array passed to the `Filters` component, it would not render. This caused issues around keeping applied filters and pinned filters in sync with each other. The component would mount with an empty `filters` and `appliedFilters` array, and then re-render once the data was available and passed down. We should not be invoking any of the internal state logic around pinned/applied filters until we have a list of filters to parse. This PR breaks out the Filters component, and puts all the stateful logic inside a new FiltersBar child component of the Filters, and this only gets rendered when `filters.length > 0`. This allows us to keep the initial pinned/applied filter state in sync and prevents and issues we were seeing around applied filters jumping around the list. ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) Spin URL: https://admin.web.filters-fix.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none&delivery_method=shipping - Confirm that the Delivery method filter pill appears when the data has loaded - Apply more filters, and then reload the page - Confirm that those filters appear when the data has loaded - Confirm that filters stay in their existing position, until deleted. Spin URL: https://admin.web.filters-fix.marc-thomas.eu.spin.dev/store/shop1/products?selectedView=all - Apply a vendor filter to the index table - Confirm that the Vendor filter pill remains in the same place when a value is selected ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
This PR takes another approach to the applied filters and pinned filters logic. In [this PR](#10566), we tried to solve for an issue on a specific index table, the Orders index, which loads the filter data asynchronously. However, I now feel this approach is wrong and we should be solving for this problem in another way. Due to how the Filters component was built before, all the logic regarding state management of pinned and applied filters was generated when the Filters component mounted, yet if there were no filters array passed to the `Filters` component, it would not render. This caused issues around keeping applied filters and pinned filters in sync with each other. The component would mount with an empty `filters` and `appliedFilters` array, and then re-render once the data was available and passed down. We should not be invoking any of the internal state logic around pinned/applied filters until we have a list of filters to parse. This PR breaks out the Filters component, and puts all the stateful logic inside a new FiltersBar child component of the Filters, and this only gets rendered when `filters.length > 0`. This allows us to keep the initial pinned/applied filter state in sync and prevents and issues we were seeing around applied filters jumping around the list. 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) Spin URL: https://admin.web.filters-fix.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none&delivery_method=shipping - Confirm that the Delivery method filter pill appears when the data has loaded - Apply more filters, and then reload the page - Confirm that those filters appear when the data has loaded - Confirm that filters stay in their existing position, until deleted. Spin URL: https://admin.web.filters-fix.marc-thomas.eu.spin.dev/store/shop1/products?selectedView=all - Apply a vendor filter to the index table - Confirm that the Vendor filter pill remains in the same place when a value is selected - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
This PR takes another approach to the applied filters and pinned filters logic. In [this PR](#10566), we tried to solve for an issue on a specific index table, the Orders index, which loads the filter data asynchronously. However, I now feel this approach is wrong and we should be solving for this problem in another way. Due to how the Filters component was built before, all the logic regarding state management of pinned and applied filters was generated when the Filters component mounted, yet if there were no filters array passed to the `Filters` component, it would not render. This caused issues around keeping applied filters and pinned filters in sync with each other. The component would mount with an empty `filters` and `appliedFilters` array, and then re-render once the data was available and passed down. We should not be invoking any of the internal state logic around pinned/applied filters until we have a list of filters to parse. This PR breaks out the Filters component, and puts all the stateful logic inside a new FiltersBar child component of the Filters, and this only gets rendered when `filters.length > 0`. This allows us to keep the initial pinned/applied filter state in sync and prevents and issues we were seeing around applied filters jumping around the list. 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) Spin URL: https://admin.web.filters-fix.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none&delivery_method=shipping - Confirm that the Delivery method filter pill appears when the data has loaded - Apply more filters, and then reload the page - Confirm that those filters appear when the data has loaded - Confirm that filters stay in their existing position, until deleted. Spin URL: https://admin.web.filters-fix.marc-thomas.eu.spin.dev/store/shop1/products?selectedView=all - Apply a vendor filter to the index table - Confirm that the Vendor filter pill remains in the same place when a value is selected - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
…spected (Shopify#10566) ### WHY are these changes introduced? Navigating through the admin this morning I noticed an issue on the Orders index, when refreshing the page with a set of filters that don't match any existing filter, that the active filters don't appear in the Filters component. See video below: https://github.com/Shopify/polaris/assets/2562596/bb170b1f-0739-4778-8474-54c1ffe29f6e It turns out that the Orders index async loads the filter data if it does not match a saved view, and returns an empty `appliedFilters` array initially, before updating it with the correct `appliedFilters`. Due to [this recently introduced change](Shopify#10429) (sorry folks 🤡) we no longer are able to react to changes to the `appliedFilters` prop because of [this line](https://github.com/Shopify/polaris/pull/10429/files#diff-f21b98ea8a1f295706a4342f1e9ce543562f6c47d6d0682367e90bb7c23961d8R167), which sets the `localPinnedFilters` state only when the component mounts. In the case of the Orders index with the parameters mentioned above, this would set 0 pinned filters, due to the `appliedFilters` being empty. Because we don't call `setLocalPinnedFilters` on changes to the `appliedFilters`, this gives us the bug outlined in the video above. The filters do get set, but the filter does not appear in the pinned list. ### WHAT is this pull request doing? This PR effectively reverts that change, and instead uses a more robust approach to ensure that FilterPills being powered by `appliedFilters` rather than `localPinnedState` do not vanish when they become unapplied (when the filter values for the filter are removed). What we want to do, is add any applied filters to the array of `localPinnedState` so that when any filter value gets removed, the FilterPill remains as an unselected FilterPill. The merchant can then click out of the Popover to fully remove the FilterPill from the Filters bar. This is achieved by a `useEffect` hook, which listens for changes to the `appliedFilterKeys` and `localPinnedFilters` arrays, and checks if all values within `appliedFilterKeys` are not currently in the `localPinnedFilters` array. If that is the case, we know that we want to add these keys to the `localPinnedFilters` array, to ensure they are added as `FilterPill`s if the `appliedFilters` value changes post-mount. ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) Video of Snapshot in use on Spinstance: https://github.com/Shopify/polaris/assets/2562596/23decb34-c423-412c-90f0-213d89834786 - Spinstance: https://admin.web.orders-pill-check.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none - Note that adding in filters that don't match a saved view, and then refreshing, the applied filters appear as FilterPills. - Storybook: https://5d559397bae39100201eedc1-kqkexazlai.chromatic.com/?path=/story/all-components-indexfilters--with-no-pinned-and-prefilled-filters&globals=polarisSummerEditions2023:true - Note that removing the values from the "Account status" filter keeps the filter pill visible, until the Popover is closed ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide --------- Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
WHY are these changes introduced?
Navigating through the admin this morning I noticed an issue on the Orders index, when refreshing the page with a set of filters that don't match any existing filter, that the active filters don't appear in the Filters component. See video below:
Screen.Recording.2023-09-19.at.16.08.57.mov
It turns out that the Orders index async loads the filter data if it does not match a saved view, and returns an empty
appliedFilters
array initially, before updating it with the correctappliedFilters
.Due to this recently introduced change (sorry folks 🤡) we no longer are able to react to changes to the
appliedFilters
prop because of this line, which sets thelocalPinnedFilters
state only when the component mounts. In the case of the Orders index with the parameters mentioned above, this would set 0 pinned filters, due to theappliedFilters
being empty. Because we don't callsetLocalPinnedFilters
on changes to theappliedFilters
, this gives us the bug outlined in the video above. The filters do get set, but the filter does not appear in the pinned list.WHAT is this pull request doing?
This PR effectively reverts that change, and instead uses a more robust approach to ensure that FilterPills being powered by
appliedFilters
rather thanlocalPinnedState
do not vanish when they become unapplied (when the filter values for the filter are removed).What we want to do, is add any applied filters to the array of
localPinnedState
so that when any filter value gets removed, the FilterPill remains as an unselected FilterPill. The merchant can then click out of the Popover to fully remove the FilterPill from the Filters bar.This is achieved by a
useEffect
hook, which listens for changes to theappliedFilterKeys
andlocalPinnedFilters
arrays, and checks if all values withinappliedFilterKeys
are not currently in thelocalPinnedFilters
array. If that is the case, we know that we want to add these keys to thelocalPinnedFilters
array, to ensure they are added asFilterPill
s if theappliedFilters
value changes post-mount.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Video of Snapshot in use on Spinstance:
Screen.Recording.2023-09-19.at.16.37.23.mov
Spinstance: https://admin.web.orders-pill-check.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none
Note that adding in filters that don't match a saved view, and then refreshing, the applied filters appear as FilterPills.
Storybook: https://5d559397bae39100201eedc1-kqkexazlai.chromatic.com/?path=/story/all-components-indexfilters--with-no-pinned-and-prefilled-filters&globals=polarisSummerEditions2023:true
Note that removing the values from the "Account status" filter keeps the filter pill visible, until the Popover is closed
🎩 checklist
README.md
with documentation changes