[IndexTable] Fix UX / jank when resizing #4033

lhoffbeck · 2021-03-04T15:48:15Z

WHY are these changes introduced?

Resolves #3991.

The IndexTable currently has some UX, jank, and performance issues when resizing. This PR aims to address a lot of these by making UX and performance fixes to make this resize buttery-smooth.

WHAT is this pull request doing?

1) Fixed sticky header alignment issues
All the sticky headers are sized on load and not resized when the window width changes.

👀 before (watch the Location column) 👀

2.mp4

👀 after (watch the Location column) 👀

6.mp4

2) Fixed first (non-sticky) column alignment issues when resizing browser
First column header doesn't resize when window does.

👀 before 👀

3.mp4

👀 after 👀

7.mp4

**3) Fixed first sticky header misalignment **
First column is out of place when you load the page and scroll down

👀 before (watch the Name column) 👀

1.mp4

👀 after 👀

8.mp4

**4) Fix flickering scrollbar **
Scrollbar flickers when resizing window large->small. The issue here was that the width of .ScrollBarContent was set by --p-scroll-bar-content-width, so when you resize smaller unless --p-scroll-bar-content-width is set on a bare-metal resize event (i.e. not by a throttled/debounced function), the scrollbar will be larger than the content when you size the window down and will flicker. Fixed this by setting this width to 0 when resizing and only actually setting --p-scroll-bar-content-width once resizing completes.

👀 before 👀

4.mp4

👀 after 👀

9.mp4

**5) Fix sticky header not resizing smoothly **
Watch the upper right corner of the sticky header. As you resize, the width of the header goes out of shape with the rest of the card, this happens because the debounce period is at a slower framerate than it should be. Fix was to update to 60FPS (and converted to use throttle instead of debounce with maxWait for clarity).

👀 before (watch upper right corner of sticky header) 👀

5.mp4

👀 after 👀

10.mp4

**6) Performance stuff **

converted the resize handler to use useCallback instead of useMemo, we really don't need to memoize anything for this
cache the column header / sticky column header DOM lookups instead of performing them on every resize event
increased the debounce period on the scrollbar resize event so it fires ~600% less frequently

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:


import React from 'react';

import {Card, IndexTable, Page, TextStyle, useIndexResourceState} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}

      <div style={{ marginBottom: 1000 }}>
        <SimpleIndexTableExample />
      </div>
    </Page>
  );
}


function SimpleIndexTableExample() {
  const customers = [
    {
      id: '3411',
      url: 'customers/341',
      name: 'Mae Jemison',
      location: 'Decatur, USA',
      orders: 20,
      amountSpent: '$2,400',
    },
    {
      id: '2561',
      url: 'customers/256',
      name: 'Ellen Ochoa',
      location: 'Los Angeles, USA',
      orders: 30,
      amountSpent: '$140',
    },
  ];
  const resourceName = {
    singular: 'customer',
    plural: 'customers',
  };

  const {
    selectedResources,
    allResourcesSelected,
    handleSelectionChange,
  } = useIndexResourceState(customers);

  const rowMarkup = customers.map(
    ({id, name, location, orders, amountSpent}, index) => (
      <IndexTable.Row
        id={id}
        key={id}
        selected={selectedResources.includes(id)}
        position={index}
      >
        <IndexTable.Cell>
          <TextStyle variation="strong">{name}</TextStyle>
        </IndexTable.Cell>
        <IndexTable.Cell>{location}</IndexTable.Cell>
        <IndexTable.Cell>{orders}</IndexTable.Cell>
        <IndexTable.Cell>{amountSpent}</IndexTable.Cell>
      </IndexTable.Row>
    ),
  );

  return (
    <Card>
      <IndexTable
        resourceName={resourceName}
        itemCount={customers.length}
        selectedItemsCount={
          allResourcesSelected ? 'All' : selectedResources.length
        }
        onSelectionChange={handleSelectionChange}
        headings={[
          {title: 'Name'},
          {title: 'Location'},
          {title: 'Order count'},
          {title: 'Amount spent'},
        ]}
      >
        {rowMarkup}
      </IndexTable>
    </Card>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

lhoffbeck · 2021-03-04T15:49:39Z

NOTE that I didn't add any test cases for this--this behavior hinges on being able to grab dimensions from the dom, which isn't possible with our test setup (afaik).

github-actions · 2021-03-04T15:50:52Z

🔴 This pull request modifies 7 files and might impact 78 other files. Because this is a larger than average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 78)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/IndexTable/IndexTable.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/IndexTable/tests/IndexTable.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/IndexTable/utilities/index.ts (total: 1)

Files potentially affected (total: 1)

/src/components/IndexTable/IndexTable.tsx

🧩 src/components/IndexTable/utilities/tests/utilities.test.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/IndexTable/utilities/utilities.ts (total: 2)

Files potentially affected (total: 2)

🧩 src/utilities/sticky-manager/sticky-manager.ts (total: 77)

Files potentially affected (total: 77)

src/components/IndexTable/IndexTable.tsx

src/utilities/sticky-manager/sticky-manager.ts

AndrewMusgrave

Love how smooth this is looking 😍

UNRELEASED.md

src/utilities/sticky-manager/sticky-manager.ts

src/components/IndexTable/IndexTable.tsx

Co-authored-by: Andrew Musgrave <[email protected]>

…ps://github.com/Shopify/polaris-react into lh/index-table-sticky-header-alignment-and-jank

AndrewMusgrave

LGTM! ✅

AndrewMusgrave · 2021-03-15T15:16:38Z

src/components/IndexTable/tests/IndexTable.test.tsx

@@ -16,6 +18,11 @@ import {ScrollContainer} from '../components';
 import {SelectionType} from '../../../utilities/index-provider';
 import {AfterInitialMount} from '../../AfterInitialMount';

+jest.mock('../utilities', () => ({
+  ...(jest.requireActual('../utilities') as any),


Why was casting needed here?

bad copy paste 🙈

Fix indextable UX / jank when resizing

9d986d0

lhoffbeck requested review from a team, maxariss and ChrisMLee and removed request for ChrisMLee March 4, 2021 15:49

lhoffbeck commented Mar 4, 2021

View reviewed changes

src/components/IndexTable/IndexTable.tsx Outdated Show resolved Hide resolved

fix linter

af0b5f7

kmdavis reviewed Mar 4, 2021

View reviewed changes

alex-page requested a review from AndrewMusgrave March 9, 2021 15:31

AndrewMusgrave reviewed Mar 10, 2021

View reviewed changes

lhoffbeck and others added 6 commits March 10, 2021 12:07

Update UNRELEASED.md

7b52d2e

Co-authored-by: Andrew Musgrave <[email protected]>

updates from PR review

4e22035

Merge branch 'lh/index-table-sticky-header-alignment-and-jank' of htt…

4a3a0d7

…ps://github.com/Shopify/polaris-react into lh/index-table-sticky-header-alignment-and-jank

Merge branch 'main' into lh/index-table-sticky-header-alignment-and-jank

bb26b7c

fix linter

69ec319

fix the linter... again...

9830439

lhoffbeck requested a review from AndrewMusgrave March 10, 2021 23:56

fix checkbox header minwidth for tiny screens

6a1ffd1

AndrewMusgrave approved these changes Mar 15, 2021

View reviewed changes

AndrewMusgrave requested a review from kmdavis March 15, 2021 15:30

remove unnecessary cast

90a107b

lhoffbeck merged commit 58a8a32 into main Mar 15, 2021

lhoffbeck deleted the lh/index-table-sticky-header-alignment-and-jank branch March 15, 2021 16:40

This was referenced Mar 18, 2021

Bump @shopify/polaris from 6.0.1 to 6.2.0 jonpulsifer/hub#81

Closed

Bump @shopify/polaris from 5.12.0 to 6.2.0 jonpulsifer/pulsifer.ca#101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IndexTable] Fix UX / jank when resizing #4033

[IndexTable] Fix UX / jank when resizing #4033

lhoffbeck commented Mar 4, 2021

lhoffbeck commented Mar 4, 2021

github-actions bot commented Mar 4, 2021 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 77)

AndrewMusgrave left a comment

AndrewMusgrave left a comment

AndrewMusgrave Mar 15, 2021

lhoffbeck Mar 15, 2021

[IndexTable] Fix UX / jank when resizing #4033

[IndexTable] Fix UX / jank when resizing #4033

Conversation

lhoffbeck commented Mar 4, 2021

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

lhoffbeck commented Mar 4, 2021

github-actions bot commented Mar 4, 2021 • edited Loading

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 77)

AndrewMusgrave left a comment

Choose a reason for hiding this comment

AndrewMusgrave left a comment

Choose a reason for hiding this comment

AndrewMusgrave Mar 15, 2021

Choose a reason for hiding this comment

lhoffbeck Mar 15, 2021

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2021 •

edited

Loading