Skip to content

Commit

Permalink
[IndexTable scroll bug] Only render BulkActionButtons when `selectMod…
Browse files Browse the repository at this point in the history
…e` is true within BulkActions (#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
  • Loading branch information
mrcthms authored Mar 13, 2024
1 parent c78f125 commit 4699bb2
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 40 deletions.
5 changes: 5 additions & 0 deletions .changeset/curly-geckos-talk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Updated `BulkActions` to only show actions when selectMode is `true`
24 changes: 0 additions & 24 deletions polaris-react/src/components/BulkActions/BulkActions.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -58,30 +58,6 @@ $bulk-actions-button-stacking-order: (
}
}

.ButtonGroupWrapper {
width: auto;
justify-content: flex-start;
max-width: 100%;
pointer-events: auto;

@media (--p-breakpoints-sm-down) {
// stylelint-disable-next-line selector-max-combinators, selector-max-type -- the first item of button group on small screen needs to fill the space
> div > div:first-child {
flex: 1 1 auto;
}
}

@media (--p-breakpoints-sm-up) {
width: auto;
justify-content: flex-start;
}

.Group-measuring & {
position: absolute;
width: auto;
}
}

.BulkActionButton {
white-space: nowrap;

Expand Down
35 changes: 19 additions & 16 deletions polaris-react/src/components/BulkActions/BulkActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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` */
selectMode?: boolean;
/** @deprecated Used for forwarding the ref. Use `ref` prop instead */
innerRef?: React.Ref<any>;
Expand Down Expand Up @@ -102,6 +102,7 @@ export const BulkActions = forwardRef(function BulkActions(
onToggleAll,
onMoreActionPopoverToggle,
width,
selectMode,
}: BulkActionsProps,
ref,
) {
Expand Down Expand Up @@ -369,22 +370,24 @@ export const BulkActions = forwardRef(function BulkActions(
<CheckableButton {...checkableButtonProps} />
{paginatedSelectAllMarkup}
</div>
<div className={styles.BulkActionsPromotedActionsWrapper}>
<InlineStack gap="100" blockAlign="center">
<div className={styles.BulkActionsOuterLayout}>
{measurerMarkup}
<div
className={classNames(
styles.BulkActionsLayout,
!hasMeasured && styles['BulkActionsLayout--measuring'],
)}
>
{promotedActionsMarkup}
{selectMode ? (
<div className={styles.BulkActionsPromotedActionsWrapper}>
<InlineStack gap="100" blockAlign="center">
<div className={styles.BulkActionsOuterLayout}>
{measurerMarkup}
<div
className={classNames(
styles.BulkActionsLayout,
!hasMeasured && styles['BulkActionsLayout--measuring'],
)}
>
{promotedActionsMarkup}
</div>
</div>
</div>
{actionsMarkup}
</InlineStack>
</div>
{actionsMarkup}
</InlineStack>
</div>
) : null}
</InlineStack>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ interface Props {
accessibilityLabel: string;
label: string;
selected: boolean;
selectMode: boolean;
}

const bulkActionProps: Props = {
Expand All @@ -56,6 +57,7 @@ const bulkActionProps: Props = {
},
],
disabled: false,
selectMode: true,
};

describe('<BulkActions />', () => {
Expand Down Expand Up @@ -317,6 +319,7 @@ describe('<BulkActions />', () => {
label: 'Label',
selected: false,
bulkActions: [],
selectMode: true,
promotedActions: [
{
title: 'button1',
Expand Down Expand Up @@ -353,6 +356,7 @@ describe('<BulkActions />', () => {
label: 'Label',
selected: false,
bulkActions: [],
selectMode: true,
promotedActions: [
{
title: 'button1',
Expand Down Expand Up @@ -385,6 +389,7 @@ describe('<BulkActions />', () => {
label: 'Label',
selected: false,
bulkActions: [],
selectMode: true,
promotedActions: [
{
title: 'button1',
Expand Down Expand Up @@ -444,6 +449,7 @@ describe('<BulkActions />', () => {
label: 'Label',
selected: false,
bulkActions: [],
selectMode: true,
promotedActions: [
{...promotedActionToBeClicked},
{
Expand Down Expand Up @@ -490,6 +496,7 @@ describe('<BulkActions />', () => {
label: 'Label',
selected: false,
bulkActions: [],
selectMode: true,
promotedActions: [
{
disabled: true,
Expand Down Expand Up @@ -555,6 +562,18 @@ describe('<BulkActions />', () => {
});
});

describe('selectMode', () => {
describe('when false', () => {
it('will not render any bulk action buttons', () => {
const bulkActions = mountWithApp(
<BulkActions {...bulkActionProps} selectMode={false} />,
);

expect(bulkActions).not.toContainReactComponent(BulkActionButton);
});
});
});

describe('deprecated props', () => {
describe('width', () => {
it('adds an inline style width if present', () => {
Expand Down

0 comments on commit 4699bb2

Please sign in to comment.