[IndexFilters] Consolidate se23 logic and styles #9938
Assignees
Comments
5 tasks
5 tasks
gwyneplaine
added a commit
that referenced
this issue
Aug 28, 2023
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Fixes #9938 ### WHAT is this pull request doing? * Consolidate se23 styles * Consolidate se23 logic * Remove custom `FilterButton` and replace it with direct invocations of the Polaris Button instead. This seems safe to do because: * even though we export `FilterButton` from the root of `IndexFilters`, we do not actually expose it directly from the `polaris-react` [entrypoint](https://github.com/Shopify/polaris/blob/2c718dc3ecf9393119cd1e08c387b648db78449a/polaris-react/src/index.ts#L190). * AFAIK, noone is reaching into our cjs/esm builds to grab`FilterButton` (see [grokt](https://grokt.shopify.io/results?q=%40shopify%2Fpolaris%2F.*%2Fcomponents%2FIndexFilters)) * Just to be sure, also doesn't look like anyone is importing `FilterButton` from @shopify/polaris either. (see [grokt](https://grokt.shopify.io/results?q=FilterButton)) * That said I can see an argument being made for providing convenience wrappers to alleviate repetitive config, so deferring to the team. 👍 Happy to revert if necessary 👍 ### 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) <!-- Give as much information as needed to experiment with the component in the playground. --> <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
sophschneider
pushed a commit
that referenced
this issue
Sep 19, 2023
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> Fixes #9938 * Consolidate se23 styles * Consolidate se23 logic * Remove custom `FilterButton` and replace it with direct invocations of the Polaris Button instead. This seems safe to do because: * even though we export `FilterButton` from the root of `IndexFilters`, we do not actually expose it directly from the `polaris-react` [entrypoint](https://github.com/Shopify/polaris/blob/2c718dc3ecf9393119cd1e08c387b648db78449a/polaris-react/src/index.ts#L190). * AFAIK, noone is reaching into our cjs/esm builds to grab`FilterButton` (see [grokt](https://grokt.shopify.io/results?q=%40shopify%2Fpolaris%2F.*%2Fcomponents%2FIndexFilters)) * Just to be sure, also doesn't look like anyone is importing `FilterButton` from @shopify/polaris either. (see [grokt](https://grokt.shopify.io/results?q=FilterButton)) * That said I can see an argument being made for providing convenience wrappers to alleviate repetitive config, so deferring to the team. 👍 Happy to revert if necessary 👍 🖥 [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) <!-- Give as much information as needed to experiment with the component in the playground. --> <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> - [ ] 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
AnnaCheba
pushed a commit
to AnnaCheba/polaris
that referenced
this issue
Apr 22, 2024
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Fixes Shopify#9938 ### WHAT is this pull request doing? * Consolidate se23 styles * Consolidate se23 logic * Remove custom `FilterButton` and replace it with direct invocations of the Polaris Button instead. This seems safe to do because: * even though we export `FilterButton` from the root of `IndexFilters`, we do not actually expose it directly from the `polaris-react` [entrypoint](https://github.com/Shopify/polaris/blob/0151ec04e4a72a657fc4f3ca53e8990c58a47cfa/polaris-react/src/index.ts#L190). * AFAIK, noone is reaching into our cjs/esm builds to grab`FilterButton` (see [grokt](https://grokt.shopify.io/results?q=%40shopify%2Fpolaris%2F.*%2Fcomponents%2FIndexFilters)) * Just to be sure, also doesn't look like anyone is importing `FilterButton` from @shopify/polaris either. (see [grokt](https://grokt.shopify.io/results?q=FilterButton)) * That said I can see an argument being made for providing convenience wrappers to alleviate repetitive config, so deferring to the team. 👍 Happy to revert if necessary 👍 ### 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) <!-- Give as much information as needed to experiment with the component in the playground. --> <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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
No description provided.
The text was updated successfully, but these errors were encountered: