-
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
[IndexTable scroll bug] Only render BulkActionButtons when selectMode
is true within BulkActions
#11723
Conversation
/snapit |
3d41fc6
to
80526cd
Compare
🫰✨ Thanks @mrcthms! Your snapshot has been published to npm. Test the snapshot by updating your yarn add @shopify/[email protected] |
@@ -67,7 +67,7 @@ export interface BulkActionsProps { | |||
buttonSize?: Extract<ButtonProps['size'], 'micro' | 'medium'>; | |||
/** Label for the bulk actions */ | |||
label?: string; | |||
/** @deprecated List is in a selectable state. No longer needed due to removal of Transition */ | |||
/** List is in a selectable state. Will only render the bulk actions when `true` */ |
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.
This is fine. My only question is did we remove this prop in some implementations and should add it back to ensure this issue doesn't happen again?
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.
I think if other implementations hard coded this value they'd see the scrollbar flash? So this should be ok, all of the logic should be handled by index table
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.
Only place outside of Polaris where this is used is https://github.com/Shopify/web/blob/8d9f81e1c7b48bd079a41823edbdb16ca16b5815/app/shared/components/ResourceListWithHeader/ResourceListWithHeader.tsx#L666, which already has the selectMode prop passed to it, so we should be good!
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.
Just one comment about adding a test to make sure bulk actions actually render in case this gets a big refactor someday
expect(bulkActions).not.toContainReactComponent(BulkActionButton); | ||
}); | ||
}); | ||
}); |
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.
Can we add another test to make sure bulk actions render when selectMode
is true?
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.
@kyledurand All the other tests in the suite have selectMode=true
as a default, and they do test for presence of BulkActionButton
, albeit indirectly. So it feels like it's a bit of a redundant test, to test specifically that they render given the other tests would fail if they didn't render when selectMode=true
@@ -67,7 +67,7 @@ export interface BulkActionsProps { | |||
buttonSize?: Extract<ButtonProps['size'], 'micro' | 'medium'>; | |||
/** Label for the bulk actions */ | |||
label?: string; | |||
/** @deprecated List is in a selectable state. No longer needed due to removal of Transition */ | |||
/** List is in a selectable state. Will only render the bulk actions when `true` */ |
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.
I think if other implementations hard coded this value they'd see the scrollbar flash? So this should be ok, all of the logic should be handled by index table
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/[email protected] ### Minor Changes - [#11677](#11677) [`f6ba2b2a8`](f6ba2b2) Thanks [@jesstelford](https://github.com/jesstelford)! - Migrated @shopify/polaris from SASS to CSS using postcss plugins - [#11723](#11723) [`4699bb229`](4699bb2) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated `BulkActions` to only show actions when selectMode is `true` - [#11727](#11727) [`c3ba6ae1b`](c3ba6ae) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Removed the responsive logic that disabled the Card bevel on mobile. Removing this until we are ready to rollout bevel changes across all components. ### Patch Changes - [#11757](#11757) [`e0ae9565c`](e0ae956) Thanks [@sophschneider](https://github.com/sophschneider)! - Added dynamicTopBarAndReframe feature flag type - [#11733](#11733) [`9c24a465c`](9c24a46) Thanks [@jesstelford](https://github.com/jesstelford)! - Convert SASS-style inline comments to CSS-style multiline comments. - [#11724](#11724) [`1543246b7`](1543246) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Updated responsive styles for `Text` component - [#11765](#11765) [`42c298ea7`](42c298e) Thanks [@jesstelford](https://github.com/jesstelford)! - Fix build performance regression from using postcss-mixins. - [#11725](#11725) [`3e011e3b6`](3e011e3) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed a bug where iOS 16 font patch wasn't added for mobile app web views - [#11763](#11763) [`e7ab4a8f5`](e7ab4a8) Thanks [@sydturn](https://github.com/sydturn)! - Fixed `IndexTable.Row` `onClick` not being called when `selectable` is `false` - [#11745](#11745) [`831a683a2`](831a683) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed bug in math.ts for popover with position cover - [#11735](#11735) [`6d8ef8c99`](6d8ef8c) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Used `Text` component to apply text styles for `Button` - [#11592](#11592) [`ad6315845`](ad63158) Thanks [@SMAKSS](https://github.com/SMAKSS)! - Passed missing `id` prop to the root element of `BlockStack` ## @shopify/[email protected] ### Minor Changes - [#11677](#11677) [`f6ba2b2a8`](f6ba2b2) Thanks [@jesstelford](https://github.com/jesstelford)! - Add `--pg-` as a disallowed CSS Custom Property prefix (these are "global" Custom Properties used within @shopify/polaris-react). ## @shopify/[email protected] ### Patch Changes - [#11754](#11754) [`f57db81df`](f57db81) Thanks [@jesstelford](https://github.com/jesstelford)! - Move migrations to v14 since the node v20 requirement will be the only change in v13 - Updated dependencies \[[`f6ba2b2a8`](f6ba2b2)]: - @shopify/[email protected] ## [email protected] ### Minor Changes - [#11720](#11720) [`97184dc80`](97184dc) Thanks [@sarahill](https://github.com/sarahill)! - Added common action guidance. - [#11690](#11690) [`c78f125c7`](c78f125) Thanks [@heyjoethomas](https://github.com/heyjoethomas)! - Updates images for icon design guidance ### Patch Changes - [#11747](#11747) [`5cff96245`](5cff962) Thanks [@sarahill](https://github.com/sarahill)! - Updated card layout patterns to match common action patterns. - Updated dependencies \[[`e0ae9565c`](e0ae956), [`9c24a465c`](9c24a46), [`f6ba2b2a8`](f6ba2b2), [`4699bb229`](4699bb2), [`c3ba6ae1b`](c3ba6ae), [`1543246b7`](1543246), [`42c298ea7`](42c298e), [`3e011e3b6`](3e011e3), [`e7ab4a8f5`](e7ab4a8), [`831a683a2`](831a683), [`6d8ef8c99`](6d8ef8c), [`ad6315845`](ad63158)]: - @shopify/[email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…e` is true within BulkActions (Shopify#11723) ### WHY are these changes introduced? Fixes https://shopify.slack.com/archives/C4Y8N30KD/p1710175615099309 Fixes a bug in the IndexTable where a near instant horizontal scrollbar appears and then disappears on every IndexTable that uses BulkActions. ### Investigation After debugging the issue, it was narrowed down to IndexTables that have a `bulkActions` / `promotedBulkActions` prop. After trying all the usual ways of trying to avoid horizontal scrollbars (setting max-widths on containing elements, setting `overflow: hidden` on containing elements, and checking for any rogue `margin` properties causing the scrollbar) and that having no effect, I had to dig deeper. Looking into the BulkActions JSX itself, as well as the paginated select all text (ruled out as a culprit), we render the More actions button – which uses the `BulkActionButton`, various promoted action buttons (which use the `BulkActionButton`), and a hidden measurer component containing a number of `BulkActionButton` components. Only when all three of these are removed do we see the issue not happening anymore. So I took a look into the `BulkActionButton` component. The component itself doesn't do anything too wild. We render a `Button` and then conditionally wrap it in a `Tooltip` depending on if it's the button which opens the ActionList containing the list of actions. Looking at the UI when the Tooltip is hovered, you can see that the Tooltip itself overlaps the right hand edge of the container. <img width="1624" alt="Screenshot 2024-03-13 at 10 35 13" src="https://github.com/Shopify/polaris/assets/2562596/9c49b854-1c5c-4a9f-9e88-90dcbac1b992"> I know that Tooltips render in a Portal, so shouldn't be part of the same DOM tree, but wondered if there's some kind of premature rendering happening where it renders in situ before being hoisted into the Portal. So I removed the Tooltip code from the component and just rendered the Button in place. I was still seeing the issue then, so it was another dead end. I next decided to remove the `Button` and replace it with a regular `button`, just out of curiosity more than anything else. This actually fixed the issue, after multiple refreshes I could not replicate the horizontal scrollbar at all. https://github.com/Shopify/polaris/assets/2562596/8951fc28-0961-4c23-aa96-e608755aaa11 But then, after reintroducing the conditional Tooltip logic with the default `button`, the issue came back again. So it appears these are all most likely red herrings to fix the actual problem. ### WHAT is this pull request doing? So going back to the drawing board, I came up with a different approach which actually solved the issue fully. We currently render the contents of the BulkActions but hide it visually until the `selectMode` within IndexTable and ResourceList is `true`, and then we change the opacity and visibility CSS properties of the containing div. Whilst we need to make sure the select all actions and checkbox are rendered due to internal logic in the ResourceList which handles focus management, there is no need for the bulk action buttons to be rendered when in the hidden state. So I used the deprecated `selectMode` prop on the `BulkActions` prop to conditionally render the measurer, actions Popover, and promoted actions. By doing this, I un-deprecated the `selectMode` prop on the Bulk Actions (q – is this okay? I think it should be but just want to check). ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces) 🗒 [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) ### 🎩 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://shopify.slack.com/archives/C4Y8N30KD/p1710175615099309
Fixes a bug in the IndexTable where a near instant horizontal scrollbar appears and then disappears on every IndexTable that uses BulkActions.
Investigation
After debugging the issue, it was narrowed down to IndexTables that have a
bulkActions
/promotedBulkActions
prop.After trying all the usual ways of trying to avoid horizontal scrollbars (setting max-widths on containing elements, setting
overflow: hidden
on containing elements, and checking for any roguemargin
properties causing the scrollbar) and that having no effect, I had to dig deeper.Looking into the BulkActions JSX itself, as well as the paginated select all text (ruled out as a culprit), we render the More actions button – which uses the
BulkActionButton
, various promoted action buttons (which use theBulkActionButton
), and a hidden measurer component containing a number ofBulkActionButton
components. Only when all three of these are removed do we see the issue not happening anymore. So I took a look into theBulkActionButton
component.The component itself doesn't do anything too wild. We render a
Button
and then conditionally wrap it in aTooltip
depending on if it's the button which opens the ActionList containing the list of actions. Looking at the UI when the Tooltip is hovered, you can see that the Tooltip itself overlaps the right hand edge of the container.I know that Tooltips render in a Portal, so shouldn't be part of the same DOM tree, but wondered if there's some kind of premature rendering happening where it renders in situ before being hoisted into the Portal. So I removed the Tooltip code from the component and just rendered the Button in place.
I was still seeing the issue then, so it was another dead end. I next decided to remove the
Button
and replace it with a regularbutton
, just out of curiosity more than anything else. This actually fixed the issue, after multiple refreshes I could not replicate the horizontal scrollbar at all.debug-index-table-issue.mov
But then, after reintroducing the conditional Tooltip logic with the default
button
, the issue came back again. So it appears these are all most likely red herrings to fix the actual problem.WHAT is this pull request doing?
So going back to the drawing board, I came up with a different approach which actually solved the issue fully. We currently render the contents of the BulkActions but hide it visually until the
selectMode
within IndexTable and ResourceList istrue
, and then we change the opacity and visibility CSS properties of the containing div. Whilst we need to make sure the select all actions and checkbox are rendered due to internal logic in the ResourceList which handles focus management, there is no need for the bulk action buttons to be rendered when in the hidden state. So I used the deprecatedselectMode
prop on theBulkActions
prop to conditionally render the measurer, actions Popover, and promoted actions. By doing this, I un-deprecated theselectMode
prop on the Bulk Actions (q – is this okay? I think it should be but just want to check).How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
🎩 checklist
README.md
with documentation changes