[BulkActions] BulkActions/SelectAllActions UX Tweaks (#7890)

### WHY are these changes introduced? Fixes Shopify/web#79604 The Bulk Actions bar can cause a visual bug whereby the bar disappears if the height of the table/resourcelist changes when the bulk actions bar is visible. This is particularly noticeable when a merchant is filtering on a table with the bulk actions selected. ### WHAT is this pull request doing? This PR updates the `use-is-sticky-bulk-actions` hook to return the `computeTableDimensions` method, which, surprisingly, calculates the dimensions of the table so that we can position the sticky bulk actions bar correctly. We need to return it from the hook so that we can invoke it in both the `IndexTable` and `ResourceList` components, and we listen out for when either the `itemCount` in the `IndexTable` changes, or the `items.length` changes in the `ResourceList`, which as we know that the table will either grow or shrink based on the new value, and the values inside our hook would have to update too as a result to correctly calculate where to place it. We have also fixed a couple of minor bugs related to the BulkActions/SelectAllActions as part of this work, namely: - The placement of the checkbox in the SelectAllActions was off by 2px to the left, so this has been adjusted. - When in select mode, if your IndexTable was sortable, it was still possible to tab through the headings even though they were invisible on the page due to the SelectAllActions. We now remove all sortable functionality from the headings when in this mode. #### Before https://user-images.githubusercontent.com/2562596/207024295-4654585e-f906-4e8d-8664-316610a2d422.mov #### After https://user-images.githubusercontent.com/2562596/207024419-b2f257ca-97a7-44a2-975f-3cbd5db5e740.mov ### Tophatting https://shop1.shopify.all-bulk-work.marc-thomas.eu.spin.dev/admin/orders?inContextTimeframe=none for a Spin link containing the Snapshot release. Note that even after updating search terms and filters for the Index Table, the Bulk Actions always persists in the correct location ### 🎩 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) - [ ] 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: Lo Kim <lo.kim@shopify.com>
Shopify · Dec 13, 2022 · 53d836d · 53d836d
1 parent ff82bdb
commit 53d836d
Show file tree

Hide file tree

Showing 11 changed files with 148 additions and 42 deletions.
diff --git a/.changeset/hip-pets-obey.md b/.changeset/hip-pets-obey.md
@@ -0,0 +1,5 @@
+---
+'@shopify/polaris': patch
+---
+
+Fixed display bug with `BulkActions` when content in the table changes at the same time the bulk actions bar is visible
diff --git a/polaris-react/src/components/BulkActions/BulkActions.tsx b/polaris-react/src/components/BulkActions/BulkActions.tsx
@@ -28,7 +28,7 @@ type TransitionStatus = 'entering' | 'entered' | 'exiting' | 'exited';
 
 const MAX_PROMOTED_ACTIONS = 2;
 
-const BUTTONS_NODE_ADDITIONAL_WIDTH = 40;
+const BUTTONS_NODE_ADDITIONAL_WIDTH = 64;
 
 export interface BulkActionsProps {
   /** List is in a selectable state */
@@ -280,6 +280,7 @@ class BulkActionsInner extends PureComponent<CombinedProps, State> {
             activator={
               <BulkActionButton
                 disclosure
+                showContentInButton={!promotedActionsMarkup}
                 onAction={this.togglePopover}
                 content={activatorLabel}
                 disabled={disabled}

diff --git a/polaris-react/src/components/BulkActions/components/BulkActionButton/BulkActionButton.tsx b/polaris-react/src/components/BulkActions/components/BulkActionButton/BulkActionButton.tsx
@@ -12,6 +12,7 @@ export type BulkActionButtonProps = {
   disclosure?: boolean;
   indicator?: boolean;
   handleMeasurement?(width: number): void;
+  showContentInButton?: boolean;
 } & DisableableAction;
 
 export function BulkActionButton({
@@ -24,6 +25,7 @@ export function BulkActionButton({
   accessibilityLabel,
   disabled,
   indicator,
+  showContentInButton,
 }: BulkActionButtonProps) {
   const bulkActionButton = useRef<HTMLDivElement>(null);
 
@@ -34,22 +36,28 @@ export function BulkActionButton({
     }
   });
 
+  const buttonContent =
+    disclosure && !showContentInButton ? undefined : content;
+
   return (
     <div className={styles.BulkActionButton} ref={bulkActionButton}>
       <Button
         external={external}
         url={url}
-        aria-label={disclosure ? content : accessibilityLabel}
+        accessibilityLabel={
+          disclosure && !showContentInButton ? content : accessibilityLabel
+        }
+        disclosure={disclosure && showContentInButton}
         onClick={onAction}
         disabled={disabled}
         size="slim"
         icon={
-          disclosure ? (
+          disclosure && !showContentInButton ? (
             <Icon source={HorizontalDotsMinor} color="base" />
           ) : undefined
         }
       >
-        {disclosure ? undefined : content}
+        {buttonContent}
       </Button>
       {indicator && <Indicator />}
     </div>

diff --git a/polaris-react/src/components/BulkActions/components/BulkActionMenu/BulkActionMenu.tsx b/polaris-react/src/components/BulkActions/components/BulkActionMenu/BulkActionMenu.tsx
@@ -24,6 +24,7 @@ export function BulkActionMenu({
         activator={
           <BulkActionButton
             disclosure
+            showContentInButton
             onAction={toggleMenuVisibility}
             content={title}
             indicator={isNewBadgeInBadgeActions}

diff --git a/polaris-react/src/components/BulkActions/hooks/use-is-bulk-actions-sticky.ts b/polaris-react/src/components/BulkActions/hooks/use-is-bulk-actions-sticky.ts
@@ -1,4 +1,4 @@
-import {useEffect, useRef, useState} from 'react';
+import {useEffect, useRef, useState, useCallback} from 'react';
 
 import {debounce} from '../../../utilities/debounce';
 
@@ -31,27 +31,27 @@ export function useIsBulkActionsSticky(selectMode: boolean) {
     hasIOSupport ? new IntersectionObserver(handleIntersect, options) : null,
   );
 
-  useEffect(() => {
-    function computeTableDimensions() {
-      const node = tableMeasurerRef.current;
-      if (!node) {
-        return {
-          maxWidth: 0,
-          offsetHeight: 0,
-          offsetLeft: 0,
-        };
-      }
-      const box = node.getBoundingClientRect();
-      const paddingHeight = selectMode ? PADDING_IN_SELECT_MODE : 0;
-      const offsetHeight = box.height - paddingHeight;
-      const maxWidth = box.width;
-      const offsetLeft = box.left;
-
-      setBulkActionsAbsoluteOffset(offsetHeight);
-      setBulkActionsMaxWidth(maxWidth);
-      setBulkActionsOffsetLeft(offsetLeft);
+  const computeTableDimensions = useCallback(() => {
+    const node = tableMeasurerRef.current;
+    if (!node) {
+      return {
+        maxWidth: 0,
+        offsetHeight: 0,
+        offsetLeft: 0,
+      };
     }
+    const box = node.getBoundingClientRect();
+    const paddingHeight = selectMode ? PADDING_IN_SELECT_MODE : 0;
+    const offsetHeight = box.height - paddingHeight;
+    const maxWidth = box.width;
+    const offsetLeft = box.left;
+
+    setBulkActionsAbsoluteOffset(offsetHeight);
+    setBulkActionsMaxWidth(maxWidth);
+    setBulkActionsOffsetLeft(offsetLeft);
+  }, [selectMode]);
 
+  useEffect(() => {
     computeTableDimensions();
 
     const debouncedComputeTableHeight = debounce(
@@ -66,7 +66,7 @@ export function useIsBulkActionsSticky(selectMode: boolean) {
 
     return () =>
       window.removeEventListener('resize', debouncedComputeTableHeight);
-  }, [tableMeasurerRef, selectMode]);
+  }, [computeTableDimensions]);
 
   useEffect(() => {
     const observer = observerRef.current;
@@ -92,5 +92,6 @@ export function useIsBulkActionsSticky(selectMode: boolean) {
     bulkActionsAbsoluteOffset,
     bulkActionsMaxWidth,
     bulkActionsOffsetLeft,
+    computeTableDimensions,
   };
 }
diff --git a/polaris-react/src/components/BulkActions/tests/BulkActions.test.tsx b/polaris-react/src/components/BulkActions/tests/BulkActions.test.tsx
@@ -228,6 +228,8 @@ describe('<BulkActions />', () => {
         const bulkActions = mountWithApp(<BulkActions {...bulkActionProps} />);
         const bulkActionButtons = bulkActions.findAll(BulkActionButton);
         expect(bulkActionButtons).toHaveLength(4);
+        expect(bulkActionButtons[0].text()).toBe('button1');
+        expect(bulkActionButtons[1].text()).toBe('button2');
         const bulkActionMenus = bulkActions.findAll(BulkActionMenu);
         expect(bulkActionMenus).toHaveLength(2);
       });

diff --git a/polaris-react/src/components/IndexTable/IndexTable.scss b/polaris-react/src/components/IndexTable/IndexTable.scss
@@ -213,6 +213,7 @@ $loading-panel-height: 53px;
 }
 
 .TableHeading {
+  background: var(--p-surface-subdued);
   padding: var(--p-space-2) var(--p-space-4);
   text-align: left;
   font-weight: var(--p-font-weight-medium);
@@ -302,7 +303,6 @@ $loading-panel-height: 53px;
 }
 
 .FirstStickyHeaderElement {
-  margin-left: calc(-1 * var(--p-space-05));
   padding-right: var(--p-space-05);
 }
 
@@ -605,7 +605,7 @@ $loading-panel-height: 53px;
   left: 0;
   // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
   right: 0;
-  padding: 0 var(--p-space-2) 0 calc(var(--p-space-2) + var(--p-space-05));
+  padding: 0 var(--p-space-2) 0 calc(var(--p-space-3) - var(--p-space-025));
   background: var(--p-surface);
 
   &.StickyTableHeader-condensed {

diff --git a/polaris-react/src/components/IndexTable/IndexTable.tsx b/polaris-react/src/components/IndexTable/IndexTable.tsx
@@ -170,8 +170,13 @@ function IndexTableBase({
     bulkActionsAbsoluteOffset,
     bulkActionsMaxWidth,
     bulkActionsOffsetLeft,
+    computeTableDimensions,
   } = useIsBulkActionsSticky(selectMode);
 
+  useEffect(() => {
+    computeTableDimensions();
+  }, [computeTableDimensions, itemCount]);
+
   const tableBodyRef = useCallback(
     (node: Element | null) => {
       if (node !== null && !tableInitialized) {
@@ -888,14 +893,15 @@ function IndexTableBase({
         <UnstyledButton
           onClick={() => handleSortHeadingClick(index, newDirection)}
           className={styles.TableHeadingSortButton}
+          tabIndex={selectMode ? -1 : 0}
         >
           {iconMarkup}
 
           {headingContent}
         </UnstyledButton>
       );
 
-      if (!sortToggleLabels) {
+      if (!sortToggleLabels || selectMode) {
         return sortMarkup;
       }
 

diff --git a/polaris-react/src/components/IndexTable/tests/IndexTable.test.tsx b/polaris-react/src/components/IndexTable/tests/IndexTable.test.tsx
@@ -11,7 +11,7 @@ import {Sticky} from '../../Sticky';
 import {Checkbox} from '../../Checkbox';
 import {Badge} from '../../Badge';
 import {Text} from '../../Text';
-import {BulkActions} from '../../BulkActions';
+import {BulkActions, useIsBulkActionsSticky} from '../../BulkActions';
 import {SelectAllActions} from '../../SelectAllActions';
 import {IndexTable, IndexTableProps} from '../IndexTable';
 import type {IndexTableSortDirection} from '../IndexTable';
@@ -35,14 +35,18 @@ jest.mock('../../../utilities/debounce', () => ({
 
 jest.mock('../../BulkActions', () => ({
   ...jest.requireActual('../../BulkActions'),
-  useIsBulkActionsSticky: () => ({
-    bulkActionsIntersectionRef: null,
-    tableMeasurerRef: null,
-    isBulkActionsSticky: false,
-    bulkActionsAbsoluteOffset: 0,
-  }),
+  useIsBulkActionsSticky: jest.fn(),
 }));
 
+function mockUseIsBulkActionsSticky(
+  args: ReturnType<typeof useIsBulkActionsSticky>,
+) {
+  const useIsBulkActionsSticky: jest.Mock =
+    jest.requireMock('../../BulkActions').useIsBulkActionsSticky;
+
+  useIsBulkActionsSticky.mockReturnValue(args);
+}
+
 const mockTableItems = [
   {
     id: 'item-1',
@@ -94,6 +98,15 @@ describe('<IndexTable>', () => {
   beforeEach(() => {
     jest.resetAllMocks();
     (getTableHeadingsBySelector as jest.Mock).mockReturnValue([]);
+    mockUseIsBulkActionsSticky({
+      bulkActionsIntersectionRef: {current: null},
+      tableMeasurerRef: {current: null},
+      isBulkActionsSticky: false,
+      bulkActionsAbsoluteOffset: 0,
+      bulkActionsMaxWidth: 0,
+      bulkActionsOffsetLeft: 0,
+      computeTableDimensions: jest.fn(),
+    });
   });
 
   it('renders an <EmptySearchResult /> if no items are passed', () => {
@@ -730,4 +743,29 @@ describe('<IndexTable>', () => {
       });
     });
   });
+
+  describe('computeTableDimensions', () => {
+    it('invokes the computeTableDimensions callback when the number of items changes', () => {
+      const computeTableDimensions = jest.fn();
+      mockUseIsBulkActionsSticky({
+        bulkActionsIntersectionRef: {current: null},
+        tableMeasurerRef: {current: null},
+        isBulkActionsSticky: false,
+        bulkActionsAbsoluteOffset: 0,
+        bulkActionsMaxWidth: 0,
+        bulkActionsOffsetLeft: 0,
+        computeTableDimensions,
+      });
+      const index = mountWithApp(
+        <IndexTable {...defaultProps} itemCount={mockTableItems.length}>
+          {mockTableItems.map(mockRenderRow)}
+        </IndexTable>,
+      );
+      expect(computeTableDimensions).toHaveBeenCalledTimes(1);
+
+      index.setProps({itemCount: 60});
+
+      expect(computeTableDimensions).toHaveBeenCalledTimes(2);
+    });
+  });
 });
diff --git a/polaris-react/src/components/ResourceList/ResourceList.tsx b/polaris-react/src/components/ResourceList/ResourceList.tsx
@@ -167,8 +167,13 @@ export const ResourceList: ResourceListType = function ResourceList<TItemType>({
     bulkActionsAbsoluteOffset,
     bulkActionsMaxWidth,
     bulkActionsOffsetLeft,
+    computeTableDimensions,
   } = useIsBulkActionsSticky(selectMode);
 
+  useEffect(() => {
+    computeTableDimensions();
+  }, [computeTableDimensions, items.length]);
+
   const defaultResourceName = useLazyRef(() => ({
     singular: i18n.translate('Polaris.ResourceList.defaultItemSingular'),
     plural: i18n.translate('Polaris.ResourceList.defaultItemPlural'),

diff --git a/polaris-react/src/components/ResourceList/tests/ResourceList.test.tsx b/polaris-react/src/components/ResourceList/tests/ResourceList.test.tsx
@@ -1,7 +1,7 @@
 import React from 'react';
 import {mountWithApp} from 'tests/utilities';
 
-import {BulkActions} from '../../BulkActions';
+import {BulkActions, useIsBulkActionsSticky} from '../../BulkActions';
 import {SelectAllActions} from '../../SelectAllActions';
 import {Button} from '../../Button';
 import {CheckableButton} from '../../CheckableButton';
@@ -18,14 +18,18 @@ import styles from '../ResourceList.scss';
 
 jest.mock('../../BulkActions', () => ({
   ...jest.requireActual('../../BulkActions'),
-  useIsBulkActionsSticky: () => ({
-    bulkActionsIntersectionRef: null,
-    tableMeasurerRef: null,
-    isBulkActionsSticky: false,
-    bulkActionsAbsoluteOffset: 0,
-  }),
+  useIsBulkActionsSticky: jest.fn(),
 }));
 
+function mockUseIsBulkActionsSticky(
+  args: ReturnType<typeof useIsBulkActionsSticky>,
+) {
+  const useIsBulkActionsSticky: jest.Mock =
+    jest.requireMock('../../BulkActions').useIsBulkActionsSticky;
+
+  useIsBulkActionsSticky.mockReturnValue(args);
+}
+
 const itemsNoID = [{url: 'item 1'}, {url: 'item 2'}];
 const singleItemNoID = [{url: 'item 1'}];
 const singleItemWithID = [{id: '1', url: 'item 1'}];
@@ -56,6 +60,16 @@ const alternateTool = <div id="AlternateTool">Alternate Tool</div>;
 const defaultWindowWidth = window.innerWidth;
 
 describe('<ResourceList />', () => {
+  mockUseIsBulkActionsSticky({
+    bulkActionsIntersectionRef: {current: null},
+    tableMeasurerRef: {current: null},
+    isBulkActionsSticky: false,
+    bulkActionsAbsoluteOffset: 0,
+    bulkActionsMaxWidth: 0,
+    bulkActionsOffsetLeft: 0,
+    computeTableDimensions: jest.fn(),
+  });
+
   describe('renderItem', () => {
     it('renders list items', () => {
       const resourceList = mountWithApp(
@@ -1270,6 +1284,31 @@ describe('<ResourceList />', () => {
       });
     });
   });
+
+  describe('computeTableDimensions', () => {
+    it('invokes the computeTableDimensions callback when the number of items changes', () => {
+      const computeTableDimensions = jest.fn();
+      mockUseIsBulkActionsSticky({
+        bulkActionsIntersectionRef: {current: null},
+        tableMeasurerRef: {current: null},
+        isBulkActionsSticky: false,
+        bulkActionsAbsoluteOffset: 0,
+        bulkActionsMaxWidth: 0,
+        bulkActionsOffsetLeft: 0,
+        computeTableDimensions,
+      });
+      const newItems = [...itemsWithID, {id: 12, url: '//shopify.com'}];
+      const resourceList = mountWithApp(
+        <ResourceList items={itemsWithID} renderItem={renderItem} />,
+      );
+
+      expect(computeTableDimensions).toHaveBeenCalledTimes(1);
+
+      resourceList.setProps({items: newItems});
+
+      expect(computeTableDimensions).toHaveBeenCalledTimes(2);
+    });
+  });
 });
 
 function noop() {}