[DataTable] Remove fixed column UX #1605

chloerice · 2019-05-30T17:32:17Z

WHY are these changes introduced?

Fixes #913 Finishes #1426

WHAT is this pull request doing?

The absolute positioning of the first column cells is no longer working across supported browsers, specifically Safari iOS (which means it doesn't work in any mobile browser in iOS). After @dleroux and I each explored other ways to accomplish the same UX with different CSS, there wasn't a clear path forward that doesn't cause other parts of the table to break.

I concluded that the fixed column experience is not worth the hacks or tradeoffs needed to fix in iOS nor the risk of future breaks. Removing the fixed first column enables removal of all the excessive complexity that went into maintaining the default table behavior of row height change etc.

Removes table collapse/fixed first column
Removes set width of first column unless truncate is true
Removes the logic that calculates and updates row heights for non-truncated tables
Removes the logic that preserves the horizontal scroll position of the table after sort of non-truncated tables

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import * as React from 'react';
import {
  Page,
  Card,
  Link,
  DataTable,
  SortDirection,
  ColumnContentType,
  TableData,
} from '../src/components';

interface State {
  truncate: boolean;
  sortedRows?: TableData[][];
}

export default class Playground extends React.Component<never, State> {
  state: State = {
    truncate: false,
  };

  sortCurrency = (
    rows: TableData[][],
    index: number,
    direction: SortDirection,
  ) => {
    return [...rows].sort((rowA: string[], rowB: string[]) => {
      const amountA = parseFloat(rowA[index].substring(1));
      const amountB = parseFloat(rowB[index].substring(1));

      return direction === 'descending' ? amountB - amountA : amountA - amountB;
    });
  };

  sortText = (rows: TableData[][], index: number, direction: SortDirection) => {
    function extractText(cellContent: TableData) {
      return typeof cellContent === 'string'
        ? cellContent.toLowerCase()
        : cellContent.props.children.toLowerCase();
    }

    return [...rows].sort((rowA: TableData[], rowB: TableData[]) => {
      const textA = extractText(rowA[index]);
      const textB = extractText(rowB[index]);

      if (textA > textB) {
        return direction === 'descending' ? -1 : 1;
      }
      if (textA < textB) {
        return direction === 'descending' ? 1 : -1;
      }

      return 0;
    });
  };

  toggleTruncate = () => {
    this.setState(({truncate}) => ({truncate: !truncate}));
  };

  handleSort = (rows: TableData[][], contentTypes: string[]) => (
    index: number,
    direction: SortDirection,
  ) => {
    if (contentTypes[index] === 'text') {
      return this.setState({
        sortedRows: this.sortText(rows, index, direction),
      });
    }
    this.setState({sortedRows: this.sortCurrency(rows, index, direction)});
  };

  render() {
    const {sortedRows, truncate} = this.state;
    const columnContentTypes: ColumnContentType[] = [
      'text',
      'numeric',
      'numeric',
      'numeric',
      'numeric',
    ];

    const initiallySortedRows = [
      [
        <Link url="https://www.example.com">Emerald Silk Gown</Link>,
        '$875.00',
        124689,
        140,
        '$122,500.00',
      ],
      [
        <Link url="https://www.example.com">Mauve Cashmere Scarf</Link>,
        '$230.00',
        124533,
        83,
        '$19,090.00',
      ],
      [
        <Link url="https://www.example.com">
          Navy Merino Wool Blazer with khaki chinos and yellow belt
        </Link>,
        '$445.00',
        124518,
        32,
        '$14,240.00',
      ],
    ];

    const rows = sortedRows ? sortedRows : initiallySortedRows;

    return (
      <Page title="Playground">
        <Card
          title="Data table with fixed column UX removed"
          actions={[
            {content: 'Toggle truncate', onAction: this.toggleTruncate},
          ]}
        >
          <DataTable
            truncate={truncate}
            columnContentTypes={columnContentTypes}
            headings={[
              'Product',
              'Price',
              'SKU Number',
              'Net quantity',
              'Net sales',
            ]}
            rows={rows}
            totals={['', '', '', 255, '$155,830.00']}
            footerContent={`Showing ${rows.length} of ${rows.length} results`}
            sortable={[true, true, false, false, true]}
            defaultSortDirection="descending"
            initialSortColumnIndex={4}
            onSort={this.handleSort(rows, columnContentTypes)}
          />
        </Card>
      </Page>
    );
  }
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility

AndrewMusgrave · 2019-06-12T15:22:13Z

Love the 🍏 : 🔴 ratio, going to 🎩 this tonight!

AndrewMusgrave

LGTM ✅ 🎉

I did notice the dots are filled in before it reaches the end, not sure if this is intentional and also not related to the PR but thought I'd mention it.

dleroux · 2019-06-17T13:22:47Z

src/components/DataTable/DataTable.scss

-  width: $fixed-column-width;
-  white-space: unset;
+  min-width: $first-column-width;
+  max-width: $first-column-width;


Do you know why min and max work but not width? And I wonder if a different min-width would work. No need to take the extra space on mobile if the column width is not needed is what I'm thinking.

Good question! I think I originally took the set width out and had no min/max, but the first column gets squished into almost nothing on small devices when the content is long and word-break: break-word is set. So I was playing with adding either a max or a min when truncated and removing the work-break, but forgot to take on or the other back out.

What I landed on is that normal table behavior should be the default, and if implementations want to have set minimum or maximum we can always enhance with a prop. So now there's only a min-width when truncate is true so that the first column is forced to truncate at that size.

dleroux · 2019-06-17T13:32:16Z

The code looks good. Just a question about the min-width. Also, any way to increase code coverage or is this mostly a Percy job?

chloerice · 2019-06-17T18:01:24Z

LGTM ✅ 🎉

I did notice the dots are filled in before it reaches the end, not sure if this is intentional and also not related to the PR but thought I'd mention it.

That's intentional, the dots indicate which columns are visible by at least 30px.

chloerice · 2019-06-17T18:04:37Z

The code looks good. Just a question about the min-width. Also, any way to increase code coverage or is this mostly a Percy job?

I've got coverage up to almost 70%. I'm all for getting to 90%, but but it seems that the other codepaths are style related conditionals that aren't testable by anything but Percy. Since this bug is keeping all tables from displaying their first column across mobile, I don't think codecov passing should block shipping this.

chloerice · 2019-06-18T11:37:20Z

src/components/DataTable/DataTable.tsx


-    const headingMarkup = (
-      <tr>
-        {headings.map((heading, headingIndex) => {


(extracted this anonymous callback into a class method like the other render{Row} callbacks, see 126 and 246)

chloerice · 2019-06-18T11:38:47Z

src/components/DataTable/DataTable.tsx

    );
  }

-  private tallestCellHeights = () => {


No more need to calculate/set/update row heights manually anymore. With the absolute positioning removed, normal table wrapping and resizing behavior is restored 🙌

chloerice · 2019-06-18T11:40:13Z

src/components/DataTable/DataTable.tsx

-    }
-  };
-
-  private setHeightsAndScrollPosition = () => {


The scroll position needed to be preserved when the row heights were being managed manually because the horizontal scroll position would be reset on rerender (most notably after clicking a sortable cell heading). This is no longer an issue.

chloerice · 2019-06-18T11:43:42Z

src/components/DataTable/DataTable.tsx

    );
  };

-  private renderFooter = () => {


Removing the absolute positioning of the first column cells made positioning the footer content seem egregious. I've changed the footer content to be rendered within a div as a sibling to table's scroll container so calculating/setting/updating the footer's height and the scroll container's bottom margin is no longer needed.

chloerice · 2019-06-18T11:44:40Z

src/components/DataTable/DataTable.tsx

            onSort(headingIndex, newSortDirection);
-
-            if (!truncate && this.scrollContainer.current) {
-              const preservedScrollPosition = {


(See https://github.com/Shopify/polaris-react/pull/1605/files#r294746700)

chloerice · 2019-06-18T11:45:51Z

src/components/DataTable/components/Cell/Cell.tsx

-
  const direction = sorted ? sortDirection : defaultSortDirection;
-  const source = direction === 'ascending' ? CaretUpMinor : CaretDownMinor;
+  const source = direction === 'descending' ? CaretDownMinor : CaretUpMinor;


(fixes icon to align with sort defaulting to ascending)

chloerice · 2019-06-18T11:56:49Z

@dleroux @AndrewMusgrave After spending tonight working on codecov, I removed a bunch of additional code that was also no longer needed because of scrapping the fixed first column. Essentially, this component is much easier to reason about without that UX 🎉

Need a second look as more deadweight code was removed and codecov was increased.

amrocha · 2019-06-18T17:09:10Z

I have written some tests for Data Table in a local branch last frideations, but haven't had the time to open a PR yet. I'll see if I can do that this friday, but don't let that stop you from shipping this :)

dleroux · 2019-06-24T13:55:16Z

src/components/DataTable/DataTable.tsx

      };

-      const columnVisibilityData = collapsedHeaderCells.map(
+      const columnVisibilityData = Array.from(headerCells).map(


Array.from is used a few times here. Judging by this https://www.measurethat.net/Benchmarks/ShowResult/59877 looks like spreading is more performant.

We have to change headerCells from a NodeList to an Array, it's not spreadable unfortunately.

You know what scratch that. I just tried it and TS doesn't yell about it anymore!

dleroux

🎩 looks good. Great job on the testing. Funny to see so much code go ahead yet more additions because of testing. Just let a comment on array.from vs spread, but looks good!

BPScott temporarily deployed to polaris-react-pr-1605 May 30, 2019 17:32 Inactive

chloerice changed the title ~~[WIP][DataTable] Remove fixed column UX~~ [DataTable] Remove fixed column UX May 30, 2019

chloerice changed the title ~~[DataTable] Remove fixed column UX~~ [WIP][DataTable] Remove fixed column UX May 31, 2019

chloerice changed the title ~~[WIP][DataTable] Remove fixed column UX~~ [DataTable] Remove fixed column UX Jun 10, 2019

chloerice force-pushed the datatable-remove-fixed-column branch from a1b7fd2 to 72e2e9e Compare June 10, 2019 17:42

BPScott temporarily deployed to polaris-react-pr-1605 June 10, 2019 17:43 Inactive

chloerice force-pushed the datatable-remove-fixed-column branch from 72e2e9e to 2498d91 Compare June 10, 2019 17:43

BPScott temporarily deployed to polaris-react-pr-1605 June 10, 2019 17:43 Inactive

chloerice requested review from AndrewMusgrave and dleroux June 10, 2019 17:45

AndrewMusgrave previously approved these changes Jun 14, 2019

View reviewed changes

dleroux reviewed Jun 17, 2019

View reviewed changes

chloerice added 6 commits June 17, 2019 20:05

start shifting language away from fixed column logic

1b4501d

fixed column => first column

63aedf5

fix small screen navigation

91f8387

collapsed => condensed

1e1fbbb

🔥 row height mgmt

8b568f5

reinstate max-width for truncated first column

e63ec8a

chloerice force-pushed the datatable-remove-fixed-column branch from 2498d91 to e63ec8a Compare June 18, 2019 01:59

BPScott temporarily deployed to polaris-react-pr-1605 June 18, 2019 02:00 Inactive

BPScott temporarily deployed to polaris-react-pr-1605 June 18, 2019 02:35 Inactive

🔥 footer position management

d46e3c5

chloerice force-pushed the datatable-remove-fixed-column branch from a9f8c66 to d46e3c5 Compare June 18, 2019 03:20

BPScott temporarily deployed to polaris-react-pr-1605 June 18, 2019 03:20 Inactive

bump code coverage

11bd7b1

BPScott temporarily deployed to polaris-react-pr-1605 June 18, 2019 11:31 Inactive

chloerice commented Jun 18, 2019

View reviewed changes

chloerice requested a review from AndrewMusgrave June 18, 2019 12:08

BPScott temporarily deployed to polaris-react-pr-1605 June 18, 2019 16:21 Inactive

dleroux reviewed Jun 24, 2019

View reviewed changes

dleroux self-requested a review June 24, 2019 13:57

dleroux approved these changes Jun 24, 2019

View reviewed changes

Array.from => ...

29ecb85

chloerice force-pushed the datatable-remove-fixed-column branch from 206181b to 29ecb85 Compare June 24, 2019 21:00

BPScott deployed to polaris-react-pr-1605 June 24, 2019 21:00 View deployment

chloerice merged commit b066474 into master Jun 25, 2019

dleroux temporarily deployed to production June 25, 2019 17:18 Inactive

chloerice mentioned this pull request Jul 12, 2019

[DataTable] 1st column blank on iOS Safari narrow screens #489

Closed

kaelig deleted the datatable-remove-fixed-column branch July 12, 2019 21:36

[DataTable] Remove fixed column UX #1605

[DataTable] Remove fixed column UX #1605

Uh oh!

Conversation

chloerice commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

AndrewMusgrave commented Jun 12, 2019

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

dleroux Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chloerice Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux commented Jun 17, 2019

Uh oh!

chloerice commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chloerice commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chloerice commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amrocha commented Jun 18, 2019

Uh oh!

dleroux Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chloerice commented May 30, 2019 •

edited

Loading

dleroux Jun 17, 2019 •

edited

Loading

chloerice Jun 17, 2019 •

edited

Loading

chloerice commented Jun 17, 2019 •

edited

Loading

chloerice commented Jun 17, 2019 •

edited

Loading

chloerice commented Jun 18, 2019 •

edited

Loading

dleroux Jun 24, 2019 •

edited

Loading