[Filters] consolidate se23 logic and styles #10178

gwyneplaine · 2023-08-23T04:38:27Z

WHY are these changes introduced?

Fixes #9934

WHAT is this pull request doing?

consolidate se23 logic
consolidate se23 styles

How to 🎩

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

gwyneplaine · 2023-08-23T04:50:03Z

polaris-react/src/components/Filters/Filters.scss

-      background: var(--p-color-bg-subdued);
-
-      #{$se23} & {
-        background: var(--p-color-bg-secondary-experimental);


Looks like a remnant from the subdued > secondary token migration.
On one hand, I think the intention here is to apply secondary-experimental instead of subdued.
On the other hand, the active background color is noticeably different to the default background color feels good (even if it is a bug)

Not a designer though, so defering to @sarahill or @bernardojoaogarcia to weigh in here.

Wrong Bernardo, I also don't have the required skills to assess this properly. 😄

Sorry about that @bernardoamc, not sure how you got tagged instead 🙏

gwyneplaine · 2023-08-23T05:01:46Z

This UI test change appears to be another case of $se23 styles (unintentionally) taking precedence over relevant non se23 styles.

In this case disabled background and border styles were being overridden by base $se23 background and border styles.

Prod styles:

PR styles:

I'm unclear as to whether or not this is intentional though (or desired), deferring to designers again 🙏 @bernardojoaogarcia @sarahill

sophschneider

I think the prod states of the "Add filter" button are messed up, according to Figma the default state is supposed to be white

Also I think the old disabled "Add filter" state with the dotted line is correct

Made a small comment about the "Clear all" button spacing changing

sophschneider · 2023-08-23T15:23:00Z

polaris-react/src/components/Filters/Filters.scss

@@ -247,32 +227,22 @@
 }

 .ClearAll {


For some reason, there is a difference in spacing for the clear all button in this PR vs. prod. See the IndexTable with filtering story

Before After

@sophschneider this looks to be a result of this change to button variants. I've changed this to monochromePlain which should be the variant value equivalent of the previous configuration plain and monochrome

sarahill · 2023-08-23T21:31:07Z

I would defer to Figma for the accurate design. There wasn't a disabled state in there so I added that for reference. https://www.figma.com/file/jLLkmo9r28hiqPvf4s90E4/Polaris-Gen-3-Components-%5Balpha%5D?type=design&node-id=68605-33198&mode=design&t=0nWQL7LChp2Emwu6-4

gwyneplaine · 2023-08-24T05:56:24Z

polaris-react/src/components/Filters/Filters.tsx

@@ -414,7 +393,7 @@ export function Filters({
          size="micro"
          onClick={handleClearAllFilters}
          removeUnderline
-          variant="tertiary"
+          variant="monochromePlain"


Previously this was monoChrome=true and plain=true, the equivalent variant value should be monochromePlain not tertiary.

gwyneplaine · 2023-08-24T06:16:05Z

polaris-react/src/components/Filters/Filters.scss

-  background: var(--p-color-bg-subdued);
-  border-radius: var(--p-border-radius-6);
-  border: var(--p-color-border-subdued) dashed var(--p-border-width-1);
+  background: var(--p-color-bg);


change this to color-bg token instead to match Figma

gwyneplaine · 2023-08-24T06:16:41Z

polaris-react/src/components/Filters/Filters.scss

    border-color: var(--p-color-border-hover);
  }

  &[aria-disabled='true'] {
-    background: var(--p-color-bg-disabled);
-    border-color: var(--p-color-border-disabled);
+    background: var(--p-color-bg-transparent-disabled-experimental);


changed to match Figma

gwyneplaine · 2023-08-24T06:18:25Z

polaris-react/src/components/Filters/components/FilterPill/FilterPill.scss

  &.disabledFilterButton {
-    background: var(--p-color-bg-disabled);
+    background: var(--p-color-bg-transparent-disabled-experimental);


changed to match Figma.

But also, will remove these entirely in a subsequent PR, since disabled FilterPills are no longer rendered

gwyneplaine · 2023-08-24T06:21:34Z

polaris-react/src/components/Filters/Filters.scss

-    background: var(--p-color-bg-disabled);
-    border-color: var(--p-color-border-disabled);
+    background: var(--p-color-bg-transparent-disabled-experimental);
+    border-color: transparent;


No border in Figma, setting to transparent to avoid layout shift.

sophschneider

It looks a lot better, thanks for fixing this!!

I noticed a few more discrepancies between figma and the current state but since that isn't related to consolidating the se23 logic I tracked the discrepancies in a separate issue that we can address in a follow up PR!

sarahill

✅

* next: (87 commits) [ButtonGroup] Removed `segmented` boolean prop and replaced with `variant` (#9997) [LegacyTabs] Consolidate se23 styles and logic (#10231) [v12] Update changesets (#10232) [LegacyFilters] Consolidate se23 styles and logic (#10217) [Banner] Change status prop to tone (#10206) Replace icon highlight color in box stories (#10226) [TopBar] Consolidate se23 styles and logic (#10221) [Filters] consolidate se23 logic and styles (#10178) [Icon] Update props (#10220) [ButtonGroup] Update variant plain prop name (#10222) [Button] Remove connectedDisclosure (#10182) [LegacyCard] Consolidate se23 styles and logic (#10207) [Navigation] Consolidate se23 logic and styles (#10213) [Frame][ContextualSaveBar] Consolidate se23 styles and logic (#10196) [OptionList] consolidate se23 styles and logic (#10177) [Page] Consolidate se23 styles and logic (#10187) [DataTable] Consolidate conditional logic (#10169) [Thumbnail] Consolidate conditional logic (#10171) [ResourceItem] Consolidate conditional logic (#10172) [FullscreenBar] Consolidate conditional logic (#10173) ...

Fixes #9934  - consolidate se23 logic - consolidate se23 styles - [Storybook](https://5d559397bae39100201eedc1-avqvvftkub.chromatic.com/?path=/story/all-components-filters--with-a-resource-list) - [Prod storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-filters--with-a-resource-list&globals=polarisSummerEditions2023:true) - [x] 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? Fixes Shopify#9934  ### WHAT is this pull request doing? - consolidate se23 logic - consolidate se23 styles ### How to 🎩 - [Storybook](https://5d559397bae39100201eedc1-avqvvftkub.chromatic.com/?path=/story/all-components-filters--with-a-resource-list) - [Prod storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-filters--with-a-resource-list&globals=polarisSummerEditions2023:true) ### 🎩 checklist - [x] 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

gwyneplaine force-pushed the v12/9934 branch from 82ac7b5 to dafb4df Compare August 23, 2023 04:42

gwyneplaine commented Aug 23, 2023

View reviewed changes

gwyneplaine requested review from sarahill, sophschneider, bernardojoaogarcia, jesstelford, sam-b-rose and laurkim August 23, 2023 05:05

sophschneider reviewed Aug 23, 2023

View reviewed changes

sophschneider mentioned this pull request Aug 23, 2023

[Filters] Styling bugs and polish #10185

Closed

gwyneplaine force-pushed the v12/9934 branch 2 times, most recently from 370669a to 3278bde Compare August 24, 2023 05:51

gwyneplaine commented Aug 24, 2023

View reviewed changes

gwyneplaine requested a review from sophschneider August 24, 2023 06:22

sophschneider approved these changes Aug 24, 2023

View reviewed changes

sarahill approved these changes Aug 24, 2023

View reviewed changes

gwyneplaine added 2 commits August 25, 2023 09:34

Filters consolidate se23 logic

aa915a5

Filters consolidate se23 styles

4b7573f

gwyneplaine force-pushed the v12/9934 branch from 3278bde to 4b7573f Compare August 24, 2023 23:34

gwyneplaine merged commit 2c718dc into next Aug 24, 2023
16 checks passed

gwyneplaine deleted the v12/9934 branch August 24, 2023 23:57

sam-b-rose mentioned this pull request Sep 19, 2023

Remove nav level rule #10568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Filters] consolidate se23 logic and styles #10178

[Filters] consolidate se23 logic and styles #10178

gwyneplaine commented Aug 23, 2023 •

edited

Loading

gwyneplaine Aug 23, 2023 •

edited by sophschneider

Loading

bernardoamc Aug 23, 2023

gwyneplaine Aug 24, 2023

gwyneplaine commented Aug 23, 2023 •

edited by sophschneider

Loading

sophschneider left a comment •

edited

Loading

sophschneider Aug 23, 2023

gwyneplaine Aug 24, 2023

sarahill commented Aug 23, 2023 •

edited

Loading

gwyneplaine Aug 24, 2023

gwyneplaine Aug 24, 2023

gwyneplaine Aug 24, 2023

gwyneplaine Aug 24, 2023

gwyneplaine Aug 24, 2023

sophschneider left a comment

sarahill left a comment

[Filters] consolidate se23 logic and styles #10178

[Filters] consolidate se23 logic and styles #10178

Conversation

gwyneplaine commented Aug 23, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

gwyneplaine Aug 23, 2023 • edited by sophschneider Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwyneplaine commented Aug 23, 2023 • edited by sophschneider Loading

sophschneider left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahill commented Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophschneider left a comment

Choose a reason for hiding this comment

sarahill left a comment

Choose a reason for hiding this comment

gwyneplaine commented Aug 23, 2023 •

edited

Loading

gwyneplaine Aug 23, 2023 •

edited by sophschneider

Loading

gwyneplaine commented Aug 23, 2023 •

edited by sophschneider

Loading

sophschneider left a comment •

edited

Loading

sarahill commented Aug 23, 2023 •

edited

Loading