Skip to content

Conversation

philschoefer
Copy link
Contributor

@philschoefer philschoefer commented Nov 10, 2022

Currently the summary row in DataTable does not properly inherit columnContentTypes given in the prop. Usually this results in no problems as 99% of the time summary rows display numerical values.

As per given logic numerical values are left aligned and string values are right aligned. In shopify/web we have a case where a string field has a summary row where alignment is now incorrect.

While it is debatable whether this should be the case in the Admin, it probably makes more sense for summary rows to inherit the columnContentType for each column, so we allow for consumers to decide how they want to use these.

WHY are these changes introduced?

https://github.com/Shopify/core-issues/issues/47369

WHAT is this pull request doing?

Before/After Screenshots Misaligned summary row due to hard-coded 'number' type in all summary rows Summary row correctly inheriting 'text' type from columnContentType definition

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Page, Card, DataTable} from '../src';

export function Playground() {
  const rows = [
    ['Emerald Silk Gown', '$875.00', 124689, 'Nov 8, 2022', '$122,500.00'],
    ['Mauve Cashmere Scarf', '$230.00', 124533, 'Nov 8, 2022', '$19,090.00'],
    [
      'Navy Merino Wool Blazer with khaki chinos and yellow belt',
      '$445.00',
      124518,
      'Nov 8, 2022',
      '$14,240.00',
    ],
  ];

  return (
    <Page title="Sales by product">
      <Card>
        <DataTable
          columnContentTypes={['text', 'numeric', 'numeric', 'text', 'numeric']}
          headings={[
            'Product',
            'Price',
            'SKU Number',
            'Last updated date',
            'Net sales',
          ]}
          rows={rows}
          totals={['', '', '', 'Nov 8, 2022', '$155,830.00']}
        />
      </Card>
    </Page>
  );
}

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2022

size-limit report 📦

Path Size
polaris-react-cjs 210.03 KB (+0.01% 🔺)
polaris-react-esm 136.15 KB (+0.01% 🔺)
polaris-react-esnext 191.36 KB (+0.01% 🔺)
polaris-react-css 41.4 KB (0%)

@philschoefer philschoefer requested a review from a team November 10, 2022 17:41
@philschoefer philschoefer self-assigned this Nov 10, 2022
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good enough to me. Do you need to /snapit before merging to make sure it doesn't break any tests / type checks in web?

@philschoefer
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

🫰✨ Thanks @philschoefer! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221110194357
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221110194357
yarn add @shopify/polaris@0.0.0-snapshot-release-20221110194357

@philschoefer
Copy link
Contributor Author

@philschoefer philschoefer merged commit bdf6fd3 into main Nov 10, 2022
@philschoefer philschoefer deleted the update-data-table-summary-row branch November 10, 2022 21:07
@github-actions github-actions bot mentioned this pull request Nov 10, 2022
laurkim pushed a commit that referenced this pull request Nov 15, 2022
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris-migrator@0.9.0

### Minor Changes

- [#7648](#7648)
[`c08780522`](c087805)
Thanks [@samrose3](https://github.com/samrose3)! - Added `animation`
properties to tokenize motion migration

### Patch Changes

- [#7690](#7690)
[`2ce4d38a2`](2ce4d38)
Thanks [@samrose3](https://github.com/samrose3)! - Ignore replacing type
imports and warn when encountering unknown props for Text component
migration


- [#7679](#7679)
[`566dc20c2`](566dc20)
Thanks [@isaacbowen](https://github.com/isaacbowen)! - Add the (missing)
case for migrating DisplayText to Text when size is not specified

## @shopify/polaris@10.12.0

### Minor Changes

- [#7684](#7684)
[`2e5f8348b`](2e5f834)
Thanks [@ananyaneogi](https://github.com/ananyaneogi)! - Update the
`CalloutCard` `title` prop to accept a ReactNode type


- [#7588](#7588)
[`c1c8f73b0`](c1c8f73)
Thanks [@rcaplanshopify](https://github.com/rcaplanshopify)! - Added
optional `captureOverscroll` prop to `Popover`


- [#6986](#6986)
[`270887fcf`](270887f)
Thanks [@danielle-dsouza](https://github.com/danielle-dsouza)! -
Clicking on the modal backdrop triggers the pressed state of the modal's
close button

### Patch Changes

- [#7693](#7693)
[`bdf6fd31d`](bdf6fd3)
Thanks [@philschoefer](https://github.com/philschoefer)! - Fixed a bug
where `DataTable` summary row would not properly inherit type defined in
`columnContentTypes` prop


- [#7578](#7578)
[`217f2f8ed`](217f2f8)
Thanks [@haskelash-shopify](https://github.com/haskelash-shopify)! -
Hide bulk actions popup when all items are deselected.


- [#7710](#7710)
[`6b915e01e`](6b915e0)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed visual spacing
regressions on `ActionList` and `Modal.Header`

## @shopify/plugin-polaris@0.0.17

### Patch Changes

- Updated dependencies
\[[`2ce4d38a2`](2ce4d38),
[`c08780522`](c087805),
[`566dc20c2`](566dc20)]:
    -   @shopify/polaris-migrator@0.9.0

## polaris.shopify.com@0.25.0

### Minor Changes

- [#7721](#7721)
[`f490f8e7c`](f490f8e)
Thanks [@jjgali](https://github.com/jjgali)! - Updated and realigned
values in Unit of measurement abbreviations table


- [#7661](#7661)
[`e1a3c62d2`](e1a3c62)
Thanks [@jjgali](https://github.com/jjgali)! - Added keywords to Naming
page.


- [#7712](#7712)
[`88e1d875f`](88e1d87)
Thanks [@jjgali](https://github.com/jjgali)! - Fixed abbrevation for
pounds from lbs to lb.

### Patch Changes

- [#7711](#7711)
[`3984f26b6`](3984f26)
Thanks [@nayeobkim](https://github.com/nayeobkim)! - Updated the related
components copy in `AlphaStack`


- [#7569](#7569)
[`ffe1e9230`](ffe1e92)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Tiles`
component guidance and examples


- [#7698](#7698)
[`f0e75bd0c`](f0e75bd)
Thanks [@nayeobkim](https://github.com/nayeobkim)! - Updated the Related
components links in Columns


- [#7713](#7713)
[`b649af84f`](b649af8)
Thanks [@nayeobkim](https://github.com/nayeobkim)! - Updated the related
components links in `Tiles`


- [#7594](#7594)
[`faf3dc646`](faf3dc6)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Box`
component guidance and examples


- [#7659](#7659)
[`d28259746`](d282597)
Thanks [@kyledurand](https://github.com/kyledurand)! - Added global
styles from deps instead of linking unpkg

- Updated dependencies
\[[`bdf6fd31d`](bdf6fd3),
[`217f2f8ed`](217f2f8),
[`2e5f8348b`](2e5f834),
[`c1c8f73b0`](c1c8f73),
[`270887fcf`](270887f),
[`6b915e01e`](6b915e0)]:
    -   @shopify/polaris@10.12.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
…`columnContentTypes` (Shopify#7693)

Currently the summary row in `DataTable` does not properly inherit
`columnContentTypes` given in the prop. Usually this results in no
problems as 99% of the time summary rows display numerical values.

As per given logic numerical values are left aligned and string values
are right aligned. In `shopify/web` we have a case where a string field
has a summary row where alignment is now incorrect.

While it is debatable whether this should be the case in the Admin, it
probably makes more sense for summary rows to inherit the
`columnContentType` for each column, so we allow for consumers to decide
how they want to use these.

### WHY are these changes introduced?

https://github.com/Shopify/core-issues/issues/47369

### WHAT is this pull request doing?

<details>
      <summary>Before/After Screenshots</summary>

<img width="1079" alt="Misaligned summary row due to hard-coded 'number'
type in all summary rows"
src="https://user-images.githubusercontent.com/4960217/201166515-86c4e6b9-8053-4563-9373-a55d557df83a.png">

<img width="1017" alt="Summary row correctly inheriting 'text' type from
columnContentType definition"
src="https://user-images.githubusercontent.com/4960217/201166523-64c099ec-6bda-4ecc-a310-a1ab985724e0.png">

</details>

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [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)

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
import React from 'react';

import {Page, Card, DataTable} from '../src';

export function Playground() {
  const rows = [
    ['Emerald Silk Gown', '$875.00', 124689, 'Nov 8, 2022', '$122,500.00'],
    ['Mauve Cashmere Scarf', '$230.00', 124533, 'Nov 8, 2022', '$19,090.00'],
    [
      'Navy Merino Wool Blazer with khaki chinos and yellow belt',
      '$445.00',
      124518,
      'Nov 8, 2022',
      '$14,240.00',
    ],
  ];

  return (
    <Page title="Sales by product">
      <Card>
        <DataTable
          columnContentTypes={['text', 'numeric', 'numeric', 'text', 'numeric']}
          headings={[
            'Product',
            'Price',
            'SKU Number',
            'Last updated date',
            'Net sales',
          ]}
          rows={rows}
          totals={['', '', '', 'Nov 8, 2022', '$155,830.00']}
        />
      </Card>
    </Page>
  );
}
```

</details>

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [x] 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
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris-migrator@0.9.0

### Minor Changes

- [Shopify#7648](Shopify#7648)
[`c08780522`](Shopify@c087805)
Thanks [@samrose3](https://github.com/samrose3)! - Added `animation`
properties to tokenize motion migration

### Patch Changes

- [Shopify#7690](Shopify#7690)
[`2ce4d38a2`](Shopify@2ce4d38)
Thanks [@samrose3](https://github.com/samrose3)! - Ignore replacing type
imports and warn when encountering unknown props for Text component
migration


- [Shopify#7679](Shopify#7679)
[`566dc20c2`](Shopify@566dc20)
Thanks [@isaacbowen](https://github.com/isaacbowen)! - Add the (missing)
case for migrating DisplayText to Text when size is not specified

## @shopify/polaris@10.12.0

### Minor Changes

- [Shopify#7684](Shopify#7684)
[`2e5f8348b`](Shopify@2e5f834)
Thanks [@ananyaneogi](https://github.com/ananyaneogi)! - Update the
`CalloutCard` `title` prop to accept a ReactNode type


- [Shopify#7588](Shopify#7588)
[`c1c8f73b0`](Shopify@c1c8f73)
Thanks [@rcaplanshopify](https://github.com/rcaplanshopify)! - Added
optional `captureOverscroll` prop to `Popover`


- [Shopify#6986](Shopify#6986)
[`270887fcf`](Shopify@270887f)
Thanks [@danielle-dsouza](https://github.com/danielle-dsouza)! -
Clicking on the modal backdrop triggers the pressed state of the modal's
close button

### Patch Changes

- [Shopify#7693](Shopify#7693)
[`bdf6fd31d`](Shopify@bdf6fd3)
Thanks [@philschoefer](https://github.com/philschoefer)! - Fixed a bug
where `DataTable` summary row would not properly inherit type defined in
`columnContentTypes` prop


- [Shopify#7578](Shopify#7578)
[`217f2f8ed`](Shopify@217f2f8)
Thanks [@haskelash-shopify](https://github.com/haskelash-shopify)! -
Hide bulk actions popup when all items are deselected.


- [Shopify#7710](Shopify#7710)
[`6b915e01e`](Shopify@6b915e0)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed visual spacing
regressions on `ActionList` and `Modal.Header`

## @shopify/plugin-polaris@0.0.17

### Patch Changes

- Updated dependencies
\[[`2ce4d38a2`](Shopify@2ce4d38),
[`c08780522`](Shopify@c087805),
[`566dc20c2`](Shopify@566dc20)]:
    -   @shopify/polaris-migrator@0.9.0

## polaris.shopify.com@0.25.0

### Minor Changes

- [Shopify#7721](Shopify#7721)
[`f490f8e7c`](Shopify@f490f8e)
Thanks [@jjgali](https://github.com/jjgali)! - Updated and realigned
values in Unit of measurement abbreviations table


- [Shopify#7661](Shopify#7661)
[`e1a3c62d2`](Shopify@e1a3c62)
Thanks [@jjgali](https://github.com/jjgali)! - Added keywords to Naming
page.


- [Shopify#7712](Shopify#7712)
[`88e1d875f`](Shopify@88e1d87)
Thanks [@jjgali](https://github.com/jjgali)! - Fixed abbrevation for
pounds from lbs to lb.

### Patch Changes

- [Shopify#7711](Shopify#7711)
[`3984f26b6`](Shopify@3984f26)
Thanks [@nayeobkim](https://github.com/nayeobkim)! - Updated the related
components copy in `AlphaStack`


- [Shopify#7569](Shopify#7569)
[`ffe1e9230`](Shopify@ffe1e92)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Tiles`
component guidance and examples


- [Shopify#7698](Shopify#7698)
[`f0e75bd0c`](Shopify@f0e75bd)
Thanks [@nayeobkim](https://github.com/nayeobkim)! - Updated the Related
components links in Columns


- [Shopify#7713](Shopify#7713)
[`b649af84f`](Shopify@b649af8)
Thanks [@nayeobkim](https://github.com/nayeobkim)! - Updated the related
components links in `Tiles`


- [Shopify#7594](Shopify#7594)
[`faf3dc646`](Shopify@faf3dc6)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Box`
component guidance and examples


- [Shopify#7659](Shopify#7659)
[`d28259746`](Shopify@d282597)
Thanks [@kyledurand](https://github.com/kyledurand)! - Added global
styles from deps instead of linking unpkg

- Updated dependencies
\[[`bdf6fd31d`](Shopify@bdf6fd3),
[`217f2f8ed`](Shopify@217f2f8),
[`2e5f8348b`](Shopify@2e5f834),
[`c1c8f73b0`](Shopify@c1c8f73),
[`270887fcf`](Shopify@270887f),
[`6b915e01e`](Shopify@6b915e0)]:
    -   @shopify/polaris@10.12.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants