-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[IndexFilters] Use context provider for IndexFilters mode state #10490
Conversation
fcb9082
to
044ca01
Compare
/snapit |
🫰✨ Thanks @mrcthms! Your snapshot has been published to npm. Test the snapshot by updating your yarn add @shopify/polaris@0.0.0-snapshot-release-20230915155701 |
52dfb76
to
deed622
Compare
/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-20230919084349 yarn add @shopify/polaris@0.0.0-snapshot-release-20230919084349 yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230919084349 yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230919084349 |
|
||
function PrimaryActionMarkup({ | ||
primaryAction, | ||
shouldOverrideDisableAction, |
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.
Why not just call this disabled
? This feels a little verbose
Edit: I see that this is slightly redundant with primaryAction.disabled
and that we won't be able to enforce custom primaryAction
components coming in as disabled
. This makes me wonder whether we should just transform the primaryAction
passed to this function before it's invoked rather than introduce an override.
const {polarisSummerEditions2023} = useFeatures(); | ||
const {isNavigationCollapsed} = useMediaQuery(); | ||
const {mode} = useIndexFiltersManager(); | ||
const shouldDisableActions = mode !== IndexFiltersMode.Default; |
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.
nit: Is the word "should" providing value here, should this just be disableActions
?
shouldShowIconOnly(isNavigationCollapsed, primaryAction), | ||
{ | ||
primary, | ||
disabled: shouldOverrideDisableAction ?? primaryAction.disabled, |
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.
Should this be an OR (i.e. ||
) rather than null coalescing? It seems shouldOverrideDisableAction
would always be a boolean, so it should never fallback to primaryAction.disabled
.
useEffect(() => { | ||
setMode(defaultMode); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [defaultMode]); |
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.
Why do we need both a useSetIndexFiltersMode
and useIndexFiltersManager
? With the solution being proposed could we not just use useIndexFiltersManager
?
Additionally, is this useEffect
necessary? It seems it's changing the behavior and meaning of defaultMode
by allowing defaultMode
to reset the state on change. I'm inclined to think it should not be doing that and should continue to be utilized as a default value, then utilize setMode
to updates by consumers.
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.
@mattkubej We don't want to remove useSetIndexFiltersMode
as this would become a breaking change to Polaris. However, I feel we could rename the useIndexFiltersManager
to useSetIndexFiltersMode
and then remove the current useSetIndexFiltersMode
from within the IndexFilters
component to remove this duplication.
As for the useEffect, we do need this as we want the useSetIndexFiltersMode
to be able to update the context when the hook initialized. I've changed this so it only runs on the mounting of the hook by removing the dependency array so it doesn't update the context on state change.
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.
Vetted the Storybook and this does address the request, but have some questions with regards to the hooks. This does seem like a reasonable approach to the problem, which should make consumers of these components ignorant of this concern.
deed622
to
bf4fa71
Compare
setMode(initialValue); | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, []); |
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.
If we're needing this to invoke setMode
with an initial value, then another consideration is to use a ref to keep track of whether this has already been invoked while mounting. If we don't care about keeping the initialValue in sync with the mode, then we could do something like this:
const hasMounted = useRef(false);
if (!hasMounted.current) {
if (initialValue) {
setMode(initialValue);
}
hasMounted.current = true;
}
/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-20230920160106 yarn add @shopify/polaris@0.0.0-snapshot-release-20230920160106 yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230920160106 yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230920160106 |
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>
…#10663) <!-- ☝️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? Reverts UX added in #10490 to Page that disables page secondary actions when IndexFilters is in filter mode. 🌀 [Spinstance ](https://admin.web.revert-page-action-disabling.chloe-rice.us.spin.dev/store/shop1/products?selectedView=all) <!-- Context about the problem that’s being addressed. --> ### WHAT is this pull request doing? <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### 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 --------- Co-authored-by: Sophie Schneider <thesophieschneider@gmail.com>
…ify#10490) ### WHY are these changes introduced? Linked to Shopify/web#96682 We want to perform actions in other components whenever we enter the non-default mode of the IndexFilters. This currently is limited to disabling the actions in the `Page` component, but could be extended in the future. ### WHAT is this pull request doing? Adding a new context manager, `IndexFiltersManager`, which allows both the IndexFilters component to use the `mode`, but also allows other components, within Polaris and also within consumers of Polaris, to react to the changes of the `mode`. <!-- ℹ️ Delete the following for small / trivial changes --> ### 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) - Spinstance: https://admin.web.if-disable-buttons.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none - Storybook: https://5d559397bae39100201eedc1-objwwxomnz.chromatic.com/?path=/story/all-components-indexfilters--wrapped-in-a-page ### 🎩 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 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 - [Shopify#10477](Shopify#10477) [`790a001cd`](Shopify@be6914d) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated semantic `color` tokens - [Shopify#10478](Shopify#10478) [`8be227e0c`](Shopify@b588f20) Thanks [@MaxCloutier](https://github.com/MaxCloutier)! - Added `allowFiltering` prop on `ActionList`, and `filterActions` prop on Page Header - [Shopify#9445](Shopify#9445) [`7be9c243a`](Shopify@4069237) 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 - [Shopify#10490](Shopify#10490) [`863f15ff2`](Shopify@07246bc) Thanks [@mrcthms](https://github.com/mrcthms)! - Add new `IndexFiltersManager` for allowing disabling of Page Header actions when in Filtering or EditingColumns mode - [Shopify#10566](Shopify#10566) [`9fed74317`](Shopify@3fb446f) Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed a bug in `Filters` where changes to the `appliedFilters` prop were not being handled ### Patch Changes - [Shopify#10404](Shopify#10404) [`5acfcec04`](Shopify@5cdd787) Thanks [@jesstelford](https://github.com/jesstelford)! - Scoped CSS variables for non-responsive props on `Tooltip`, `RangeSlider`, `ProgressBar`, and `HorizontalStack`. - [Shopify#10582](Shopify#10582) [`3efbc1b4e`](Shopify@d47089b) Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed the focus states of actions within the Page Header component - [Shopify#10492](Shopify#10492) [`d5ff72dec`](Shopify@8057862) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated Storybook stories to be wrapped with an empty FrameContext.Provider - Updated dependencies \[[`fe1aac1b5`](Shopify@a4f7ed7), [`790a001cd`](Shopify@be6914d), [`63cf3ad24`](Shopify@d4e6bd4), [`120e96eae`](Shopify@fe98b0b)]: - @shopify/polaris-tokens@7.10.0 ## @shopify/polaris-tokens@7.10.0 ### Minor Changes - [Shopify#10465](Shopify#10465) [`fe1aac1b5`](Shopify@a4f7ed7) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated private primitive `colors` - [Shopify#10477](Shopify#10477) [`790a001cd`](Shopify@be6914d) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated semantic `color` tokens - [Shopify#10600](Shopify#10600) [`63cf3ad24`](Shopify@d4e6bd4) Thanks [@lgriffee](https://github.com/lgriffee)! - Added public primitive and semantic `shadow` token scales ### Patch Changes - [Shopify#10485](Shopify#10485) [`120e96eae`](Shopify@fe98b0b) Thanks [@lgriffee](https://github.com/lgriffee)! - Updated public primitive `base` and `light-uplift` theme scales ## @shopify/polaris-migrator@0.22.5 ### Patch Changes - [Shopify#10575](Shopify#10575) [`ea6b54284`](Shopify@9761c8a) Thanks [@aveline](https://github.com/aveline)! - Handled `buttonFrom` and `buttonsFrom` functions in `Button` migration - Updated dependencies \[[`fe1aac1b5`](Shopify@a4f7ed7), [`790a001cd`](Shopify@be6914d), [`63cf3ad24`](Shopify@d4e6bd4), [`120e96eae`](Shopify@fe98b0b)]: - @shopify/polaris-tokens@7.10.0 - @shopify/stylelint-polaris@14.0.5 ## @shopify/stylelint-polaris@14.0.5 ### Patch Changes - Updated dependencies \[[`fe1aac1b5`](Shopify@a4f7ed7), [`790a001cd`](Shopify@be6914d), [`63cf3ad24`](Shopify@d4e6bd4), [`120e96eae`](Shopify@fe98b0b)]: - @shopify/polaris-tokens@7.10.0 ## polaris.shopify.com@0.57.8 ### Patch Changes - [Shopify#10605](Shopify#10605) [`9748b0838`](Shopify@a21aca4) Thanks [@laurkim](https://github.com/laurkim)! - Updated logic for rendering `color` custom property previews in `TokenList` - [Shopify#10573](Shopify#10573) [`da09e0b8c`](Shopify@439fedc) Thanks [@kyledurand](https://github.com/kyledurand)! - Updated copy url to change browser url - Updated dependencies \[[`fe1aac1b5`](Shopify@a4f7ed7), [`5acfcec04`](Shopify@5cdd787), [`790a001cd`](Shopify@be6914d), [`8be227e0c`](Shopify@b588f20), [`63cf3ad24`](Shopify@d4e6bd4), [`7be9c243a`](Shopify@4069237), [`863f15ff2`](Shopify@07246bc), [`3efbc1b4e`](Shopify@d47089b), [`d5ff72dec`](Shopify@8057862), [`120e96eae`](Shopify@fe98b0b), [`9fed74317`](Shopify@3fb446f)]: - @shopify/polaris-tokens@7.10.0 - @shopify/polaris@11.20.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…Shopify#10663) <!-- ☝️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? Reverts UX added in Shopify#10490 to Page that disables page secondary actions when IndexFilters is in filter mode. 🌀 [Spinstance ](https://admin.web.revert-page-action-disabling.chloe-rice.us.spin.dev/store/shop1/products?selectedView=all) <!-- Context about the problem that’s being addressed. --> ### WHAT is this pull request doing? <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### 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 --------- Co-authored-by: Sophie Schneider <thesophieschneider@gmail.com>
WHY are these changes introduced?
Linked to https://github.com/Shopify/web/issues/96682
We want to perform actions in other components whenever we enter the non-default mode of the IndexFilters. This currently is limited to disabling the actions in the
Page
component, but could be extended in the future.WHAT is this pull request doing?
Adding a new context manager,
IndexFiltersManager
, which allows both the IndexFilters component to use themode
, but also allows other components, within Polaris and also within consumers of Polaris, to react to the changes of themode
.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
🎩 checklist
README.md
with documentation changes