[DataTable] Added ability to control location of Totals row

[mitchmaps]

TL;DR:

Added a prop to DataTable which allows control over placement of the Totals row.

Why are these changes introduced?

Addresses #2100

Currently the DataTable component displays the Totals row at the top of the table which is defintely a valid place to display that information but in some cases it makes more sense from a UX perspective to have that information at the bottom of the table. This creates the visual flow as your eye goes down each column and subconsciously starts to add up the different values. Having the option in Polaris to do both will make the component more flexible and allow devs and designers alike to customize it to fit the context that it's being implemented in.

What is this pull request doing?

This PR introduces an additional optional prop to the DataTable component called footerTotals (could definitely find a better name) that, when set to true will place the existing Totals row and it's data supplied in other props at the bottom of the data table. Introducing these changes this way makes them backwards compatible with existing implementations of the DataTable because if the prop hasn't been supplied then then the default behaviour will be preserved.

Default rendering:

<DataTable
  columnContentTypes={[
    'text',
    'numeric',
    'numeric',
    'numeric',
    'numeric',
  ]}
  headings={[
    'Product',
    'Price',
    'SKU Number',
    'Net quantity',
    'Net sales',
  ]}
  rows={rows}
  totals={['', '', '', 255, '$155,830.00']}
/>

With footerTotals rendering:

<DataTable
  columnContentTypes={[
    'text',
    'numeric',
    'numeric',
    'numeric',
    'numeric',
  ]}
  headings={[
    'Product',
    'Price',
    'SKU Number',
    'Net quantity',
    'Net sales',
  ]}
  rows={rows}
  totals={['', '', '', 255, '$155,830.00']}
  footerTotals
/>

If you have any suggestions for a better prop name than footerTotals 🦶🔢, let me know 😂

[danrosenthal]

Before moving into review, I'd like to see some tests for this feature. Can you make sure code coverage for your changes is at 90% or better?

[mitchmaps]

Sorry for the early re-request, I didn't see the comment about test coverage! @danrosenthal

[mitchmaps]

@danrosenthal I'm not sure how I'd be able to test these sort of changes because the only effect this new prop has on the component is a visual and we wouldn't normally test for that case. What's your take on this?

[danrosenthal]

That's a great point @mitchmaps, we don't want to test the visuals on this, we just want to make sure that all code paths are covered by tests.

Codecov

Looking at the coverage report, we see that each of these lines of code is only half covered (yellow === partial coverage, green === full coverage, red === no coverage), meaning only one outcome of the ternary is covered. By passing the footer totals prop in any test, we will get code coverage over these lines. We don't even necessarily need to assert the outcome of passing that prop. We just want to make sure that when our tests run, all possible code paths are executed.

In this case, your test could simply be that the totals markup is rendered somewhere in the component when footer totals is passed, visual regression testing will handle whether it is rendered in the right place. Alternatively, you could simply find an existing test, pass the prop in that test, and assert nothing about its behavior. This will execute all code paths, and create full coverage on those lines of code.

[danrosenthal]

Nice work on that test, this is looking great!

I'm curious if you considered showFooterTotals instead of showTotalsFooter. To me it feels more natural to say, but I don't feel strongly it's the right choice.

Looking at your test brought a markup concern to my attention. It looks like the totals markup is being rendered in a thead element. A tfoot element would be more semantic to use as a wrapper (edit) for footer totals, since they now fall at the bottom of the table. You'd also want to test for the presence of this conditionally rendered markup, which you could do by using it to find() the element and assert its length.

[mitchmaps]

I did consider the other name for the action oriented prop but I figured that showFooterTotals made it sound like we were rendering total information about the footer rather than just rendering existing info within the footer. showTotalsFooter, to me sounds more like existing information being rendered in a different location but I agree isn't the best name. Maybe even just renaming it to showTotalsInFooter would make it more clear. What do you think? @danrosenthal

[danrosenthal]

showTotalsInFooter 💯

[danrosenthal]

Pinging @sarahill to make sure design is looped in on review, and for UIKit concerns.

This is good to go on my end, but please wait to ship until we've got design approval. Thanks for working with me on getting this just right 😄

Also be sure to use a tfoot instead of thead for footer totals.

I recommended the wrong thing. Will revise in a comment.

[danrosenthal]

You'll just want to make sure the totalsMarkup is wrapped in a <tfoot> when it's in the footer. This'll mean you'll need to also update the find in your test.

[danrosenthal]

This looks great code-wise. Would love to see an example of this prop added to the README before it ships. Otherwise, this LGTM!

@sarahill, once the example is in place, would you be able to give this a look design-wise?

[sarahill]

This is looking good!

Noticed one thing with the border when the row is moved to the bottom. There's a bottom border on that row, can we change it to border-top when it's on the bottom?

border-bottom (existing)	border-top (expected)

[danrosenthal]

Good eye @sarahill!

[mitchmaps]

@sarahill , that suggestion makes a lot of sense. I've implemented the changes now:

[github-actions]

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	6
Files potentially affected	1

Details

All files potentially affected (total: 1)

• /src/components/DataTable/DataTable.tsx

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/DataTable/DataTable.scss (total: 1)

Files potentially affected (total: 1)

• /src/components/DataTable/DataTable.tsx

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

Files potentially affected (total: 0)

📄 src/components/DataTable/README.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/DataTable/components/Cell/Cell.tsx (total: 1)

Files potentially affected (total: 1)

• /src/components/DataTable/DataTable.tsx

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

Files potentially affected (total: 0)

---

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

[sarahill]

LGTM! Nice work 🚀

[vict-shevchenko]

Thanks a lot, we will now consume these changes in our project.