[FiltersBar] Fix vanishing filters #11417
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-20240109112719yarn add @shopify/polaris@0.0.0-snapshot-release-20240109112719yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20240109112719yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20240109112719 |
sophschneider
added
Uplift polish
|
@sophschneider That's expected now, the issue we're fixing in this PR is what is seen in the video below, where the filters vanish when clearing. The filters now appear in the Popover behind the "Add filters" button when cleared. We intentionally changed functionality so that if a filter is interacted with and cleared, we unpin it, even if it was pinned when the component mounts. This was due to the filters bar feeling cluttered with empty filters when an IndexFilters with multiple pinned filters was used. Screen.Recording.2024-01-09.at.17.15.57.mov |
There was a problem hiding this comment.
Hey @mrcthms , thanks for looking into this! 🙏 Our team @Shopify/variants-foundations-frontend initially discovered three issues that might have the same underlying problem, here's a breakdown:
Issue 1
When the popover is opened and then clicked outside, the filter disappears and cannot be reopened without reloading the page.
Video
09-02-bd4g7-6ouc0.mp4
Issue 2
When a filter is set and then cleared individually, it also disappears without the ability to reopen it without reloading the page.
Video
09-03-gsoav-vhc0l.mp4
Issue 3
The "Clear all" button is displayed even when there are no filters to clear, for example, when a filter is set and then unset.
Video
09-03-gsoav-vhc0l.mp4
Thanks for providing the context. It seems that the issues with removing pinned filters in 1 and 2 were actually intentional design choices. However, the problem lies in the inability to reopen those filters after they have been removed which is being fixed by this PR. This PR also seems to have resolved issue 3 as well.
Does that sound accurate to you?
Could you please restart the spin instance as it seems to be dead 😨
|
/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-20240110081218yarn add @shopify/polaris@0.0.0-snapshot-release-20240110081218yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20240110081218yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20240110081218 |
|
@tatianau Yes, that's correct on all three counts. I've updated the Spin instance with the latest snapshot which hides the Clear all button if there's nothing to clear, so you should see that on the Spin. Also, with the new Spin rules around more aggressive suspending of Spin instances, you may find this link handy, where you can resume the spinstance yourself if it goes down by the time you see this |
|
I find this behaviour really bizarre from a UX standpoint ... "clear" implies clearing the selection, but to have the entire filter disappear feels really jarring. Even more so when it's just a mouseclick out of the popover. This is particularly weird for filters we've pre-pinned. Is there any chance we could revisit this UX decision? |
Summary
This summary was generated using OpenAI's gpt-4 with a temperature of 0.5. 🧵 Slack Thread
Tatiana Uspenskaia archived this conversation from |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
/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-20240112094043yarn add @shopify/polaris@0.0.0-snapshot-release-20240112094043yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20240112094043yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20240112094043 |
There was a problem hiding this comment.
Thank you so much for fixing pinned filters @mrcthms! Tophatted in storybook and can confirm:
- ✅ Not removing pre-pinned filter on popover dismiss is fixed
- ✅ Not removing pre-pinned filters on "clear all" is fixed
- ✅ Not removing pre-pinned filter when clicking "clear" within that filter popover is fixed
- ✅ "Clear all" is only rendered when filter(s) applied and removed when there is nothing to clear
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@12.10.0 ### Minor Changes - [#11417](#11417) [`cea2dd22b`](cea2dd2) Thanks [@mrcthms](https://github.com/mrcthms)! - [FiltersBar] Fixed bug where filters would disappear from the FiltersBar when clicking the Clear all button ### Patch Changes - [#11446](#11446) [`6e40eca5e`](6e40eca) Thanks [@mrcthms](https://github.com/mrcthms)! - [Icons] Fixed references to incorrect icon imports that were causing Storybook to break ## polaris.shopify.com@0.61.9 ### Patch Changes - [#11446](#11446) [`6e40eca5e`](6e40eca) Thanks [@mrcthms](https://github.com/mrcthms)! - [Icons] Fixed references to incorrect icon imports that were causing Storybook to break - Updated dependencies \[[`6e40eca5e`](6e40eca), [`cea2dd22b`](cea2dd2)]: - @shopify/polaris@12.10.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
### WHY are these changes introduced? Fixes Shopify/web#114283 We currently have a bug in the FiltersBar component, where if all filters are pinned on mount, and a merchant either clicks the "Clear all" button, or clears a filter individually, then that filter vanishes until the merchant refreshes the page. The cause of this was some missing logic to determine whether to show the Add filter button. Previously, it only checked for pinned filters on mount, and not for changes to the pinned filter state. ### How to 🎩 Spin URL: https://admin.web.vanishing-filters.marc-thomas.eu.spin.dev/store/shop1/collections?selectedView=all Story instance: https://5d559397bae39100201eedc1-epgxutgfws.chromatic.com/?path=/story/all-components-filters--with-all-filters-pinned ### 🎩 checklist - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [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
WHY are these changes introduced?
Fixes https://github.com/Shopify/web/issues/114283
We currently have a bug in the FiltersBar component, where if all filters are pinned on mount, and a merchant either clicks the "Clear all" button, or clears a filter individually, then that filter vanishes until the merchant refreshes the page.
The cause of this was some missing logic to determine whether to show the Add filter button. Previously, it only checked for pinned filters on mount, and not for changes to the pinned filter state.
How to 🎩
Spin URL: https://admin.web.vanishing-filters.marc-thomas.eu.spin.dev/store/shop1/collections?selectedView=all
Story instance: https://5d559397bae39100201eedc1-epgxutgfws.chromatic.com/?path=/story/all-components-filters--with-all-filters-pinned
🎩 checklist
README.mdwith documentation changes