[VF][Polaris] IndexTable row borders is configurable #11676

oksanashopify · 2024-02-29T21:20:05Z

Resolves first acceptance criteria from https://github.com/Shopify/web/issues/110163 and adds conditional borders

WHY are these changes introduced?

While woking on Variants card, we decided to provide better visualization for nested rows and remove borders the achieve the following design:

WHAT is this pull request doing?

Added borderless property to the Row component to specify whether the row should have border-top, especially useful for nested rows
Adds new stories:

indextable--default-borderless:

indextable--with-nested-rows-borderless

indextable--condensed-borderless

indextable--with-sub-headers-borderless
(have some issues with first two columns sticky, we have white spaces for first two cells in each row)

Updates Polaris website with code example:

Fixes an issue with doubled border for the .Polaris-IndexTable__TableRow.Polaris-IndexTable__TableRow--subheader .Polaris-IndexTable__TableCell:first-child and IndexTable__TableRow.Polaris-IndexTable__TableRow--subheader .Polaris-IndexTable__TableCell:last-child. Commit
Before:

After:

How to 🎩

Pull fresh web and apply the latest snapshot
Go to any IndexTable and try to hide borders by applying

<IndexTable.Row
     ...
      borderless
     ...
    >

Test new stories here:

Pull this branch, run yarn && yarn build and yarn turbo run dev --filter=polaris.shopify.com to tophat changes on polaris.shopify.com
You can also see on my spin how it works for Variants card on PDP, here
Draft web PR that implements latest snapshot on web: PR

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

Tested a snapshot
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

oksanashopify · 2024-02-29T21:32:25Z

/snapit

oksanashopify · 2024-02-29T21:35:05Z

/snapit

github-actions · 2024-02-29T21:38:07Z

🫰✨ Thanks @oksanashopify! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

yarn add @shopify/polaris@0.0.0-snapshot-20240229213741

oksanashopify · 2024-03-06T16:48:08Z

/snapit

github-actions · 2024-03-06T16:51:12Z

🫰✨ Thanks @oksanashopify! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

yarn add @shopify/polaris@0.0.0-snapshot-20240306165043

oksanashopify · 2024-03-07T17:15:06Z

/snapit

github-actions · 2024-03-07T17:18:13Z

🫰✨ Thanks @oksanashopify! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

yarn add @shopify/polaris@0.0.0-snapshot-20240307171744

.changeset/purple-olives-grin.md

tatianau

Left some comments on:

Component API
Chosen approach resulting in different row height and partially enforcing divider on certain elements
Regression on mobile

tatianau · 2024-03-07T17:40:19Z

polaris-react/src/components/IndexTable/IndexTable.module.scss

@@ -159,6 +159,10 @@ $loading-panel-height: 53px;
    cursor: auto;
  }

+  &.TableRow-borderless:not(:first-child) {
+    border-top: 0;


Removing the border will affect the row height making it inconsistent across rows with different divider values. Maybe instead of removing the border it's worth making it transparent instead which would keep the same element dimensions and remove the visual border?

Thank you for pointing that out! Yes, I think border-top: transparent; should work better

Trying to hide borders via applying transparent border brings implications with sticky cells, comment here.

With setting transparent color for borders we have to:

add styles to make borders the same colour as background for all sticky cells that do not have white background (rows with status, subheaders). Without colored borders sticky cell has white spaces around it. I was trying to figure out how to eliminate this, but even chatGPT says it is likely due to the way the position: sticky property works in conjunction with table elements., border-collapse: collapse;, - didn't help as well.

when it's feasible to add all those additional styles for sticky cells, I think it will bring implications to support those style, as developers have to take into account adding new types of rows

Trying to hide borders via applying border the same background color brings implications with sticky cells for row with status, subheaders, as they all needs owerrides for sticky cells borders. Moreover colored border will overlap with vertical border when first cells become sticky, video:

Screen.Recording.2024-03-15.at.8.24.24.PM.mov

Additionally, in all these cases we have to handle selected cells borders (and maybe something else will come up)

I also checked how it works in Bootstrap toolkit, for example, and they also set border: 0 to hide borders: https://getbootstrap.com/docs/4.1/content/tables/#borderless-table.

So seems like setting border: 0 is the solution that is easy to maintain and support.

@tatianau do you think border: 0 is still no way to go for us? Let's see what Polaris team thinks as well?

You can also see on my spin how it works for Variants card on PDP, here

Draft web PR that implements latest snapshot on web: https://github.com/Shopify/web/pull/120568

I also was trying to fix the initial state for sticky cells, and it seems we can do it on the table level by applying

border-collapse: separate; border-spacing: 0;

instead of

border-collapse: collapse;

But that's affects all tables spacings and borders, here is a sandbox , where we can see that table with border-collapse: separate; takes less space and borders look different. We see that difference because border-collapse: separate; border-spacing: 0; treats each cell's border individually with no spacing between them, while border-collapse: collapse; collapses the borders of adjacent cells into a single border with no spacing between them.

I agree, there's a pretty noticeable layout shift that happens when you toggle between pages that have a border and are borderless with the border: 0 solution.

I think it makes sense to modify the styles to keep the same border width with the transparent color:

&.TableRow-borderless { border-top: var(--p-border-width-025) solid transparent; border-bottom: var(--p-border-width-025) solid transparent; }

And then in the TableRow-subheader class, override the border-color for TableRow-borderless and set it to match border-color: var(--p-color-bg-surface-secondary) to fix the issue of the white space in the subheader example in your story. This solution should still work to resolve the issue of double borders in your other screenshots.

polaris-react/src/components/IndexTable/IndexTable.module.scss

.changeset/purple-olives-grin.md

polaris-react/src/components/IndexTable/IndexTable.module.scss

oksanashopify · 2024-03-12T00:40:25Z

polaris-react/src/components/IndexTable/IndexTable.stories.tsx

@@ -4663,43 +4884,255 @@ export function WithSubHeaders() {
  );
 }

-export function WithPagination() {


This diff looks weird as I didn't touch WithPagination story

oksanashopify · 2024-03-12T00:41:16Z

polaris-react/src/components/IndexTable/IndexTable.module.scss

@@ -260,9 +264,11 @@ $loading-panel-height: 53px;
      font-weight: var(--p-font-weight-medium);
      font-size: var(--p-font-size-300);
      background-color: var(--p-color-bg-surface-secondary);
-      border-top: var(--p-border-width-025) solid var(--p-color-border);


Fixes double borders for rowType="subheader" (Before and After screenshots are posted in PR description).
@laurkim could you please check if removing these borders doesn't break anything. From what I can see they were added with this PR https://github.com/Shopify/polaris/pull/10683/files and than were updated with the following one: #10741

Thank you!

I think now I noticed what your styles were fixing, for some reasons there are white spaces around two first cells in each row. I wonder if there is another way to fix it rather than adding borders 🤔

Screen.Recording.2024-03-12.at.9.02.42.AM.mov

oksanashopify · 2024-03-12T00:51:48Z

/snapit

github-actions · 2024-03-12T00:55:04Z

🫰✨ Thanks @oksanashopify! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

yarn add @shopify/polaris@0.0.0-snapshot-20240312005434

oksanashopify · 2024-03-15T02:56:15Z

/snapit

oksanashopify · 2024-03-15T18:26:12Z

converting back to draft to resolve conflicts

added story with nested rows added website example, added new story renamed divider to the borderless, updated stories, code example updated patch description updated website example refactored styles to use transparent token applied same bg color for border color instead of making it transparent moved back to border 0

oksanashopify · 2024-03-15T22:04:56Z

/snapit

github-actions · 2024-03-15T22:08:13Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240315220634

yarn add @shopify/polaris@0.0.0-snapshot-20240315220634

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240315220634

oksanashopify · 2024-03-16T02:21:41Z

/snapit

github-actions · 2024-03-16T02:25:18Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240316022351

yarn add @shopify/polaris@0.0.0-snapshot-20240316022351

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240316022351

oksanashopify · 2024-03-18T14:30:10Z

polaris-react/src/components/IndexTable/IndexTable.module.css

@@ -256,9 +260,19 @@
      font-weight: var(--p-font-weight-medium);
      font-size: var(--p-font-size-300);
      background-color: var(--p-color-bg-surface-secondary);
-      border-top: var(--p-border-width-025) solid var(--p-color-border);


Seems we can safely remove these borders. I would fix the following issue with doubled borders:

oksanashopify · 2024-03-18T16:00:44Z

/snapit

github-actions · 2024-03-18T16:04:11Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240318160236

yarn add @shopify/polaris@0.0.0-snapshot-20240318160236

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240318160236

sam-b-rose · 2024-03-20T00:00:22Z

polaris-react/src/components/IndexTable/components/Row/Row.tsx

@@ -39,6 +39,8 @@ export interface RowProps {
   * @default "Select {resourceName}"
   */
  accessibilityLabel?: string;
+  /** Whether the row should have border-top */
+  borderless?: boolean;


Suggested change

borderless?: boolean;

border?: 'none';

Can we simplify the prop name to border with the default being true? When no border is desired, the usage would be border={false}. Using border is more aligned with the CSS semantic.

Sorry, I know that will also affect the class names and stories 😅

After thinking more about it, using a string would be more flexible for future needs. Could we use border="none"?

Thank you for your feedback, @sam-b-rose !
In case with borderless we add TableRow-borderless class and remove borders with css.
If you suggest border?: boolean; or border="none", how this should be styled? Should we by default add TableRow-border class?
With border="none" what other values it should have, border="yes"?

Thank you!

Hey @oksanashopify! Sam is suggesting string typing so that if there is a need in the future for other variants of border styling, they can simply be unioned with "none" as needed (like "none" | "some other option"). For the classname, you can update to follow style variant naming convention TableRow-borderNone.

@sam-b-rose @oksanashopify what do you think of aligning the prop with Box?

/** The border-top color of the row * @default 'secondary' */ borderColor?: 'secondary' | 'transparent';

(IndexTable.css @ line 158)

.TableRow { background-color: var(--p-color-bg-surface); cursor: pointer; border-width: var(--p-border-width-025); border-style: solid; &.TableRow-unclickable { cursor: auto; } &:first-child { border-color: var(--p-color-border); } &.borderColorSecondary { border-color: var(--p-color-border-secondary); } &.borderColorTransparent { border-color: transparent; }

chloerice · 2024-04-15T19:31:41Z

polaris-react/src/components/IndexTable/components/Row/Row.tsx

@@ -55,6 +57,7 @@ export const Row = memo(function Row({
  selectionRange,
  rowType = 'data',
  accessibilityLabel,
+  borderless = false,


Suggested change

borderless = false,

borderColor = 'secondary',

chloerice · 2024-04-15T19:33:02Z

polaris-react/src/components/IndexTable/components/Row/Row.tsx

@@ -122,6 +125,7 @@ export const Row = memo(function Row({
    !selectable &&
      !primaryLinkElement.current &&
      styles['TableRow-unclickable'],
+    borderless && styles['TableRow-borderless'],


Suggested change

borderless && styles['TableRow-borderless'],

borderColor && styles[variationName('borderColor', borderColor)],

translation-platform · 2024-04-19T21:55:05Z

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

oksanashopify · 2024-07-10T18:55:15Z

As we not getting any requests to make these rows configurable anymore, we are closing this PR for now

oksanashopify self-assigned this Feb 29, 2024

oksanashopify marked this pull request as ready for review February 29, 2024 21:34

oksanashopify requested review from m4thieulavoie, brendanatshopify and tatianau March 7, 2024 17:26

oksanashopify commented Mar 7, 2024

View reviewed changes

.changeset/purple-olives-grin.md Outdated Show resolved Hide resolved

tatianau reviewed Mar 7, 2024

View reviewed changes

oksanashopify marked this pull request as draft March 11, 2024 19:10

oksanashopify changed the title ~~[VF][Polaris] Displaying IndexTable row dividers is configurable~~ [VF][Polaris] IndexTable row borders is configurable Mar 11, 2024

oksanashopify commented Mar 12, 2024

View reviewed changes

oksanashopify marked this pull request as ready for review March 12, 2024 00:51

oksanashopify requested review from laurkim, tatianau and aaronccasanova and removed request for m4thieulavoie, brendanatshopify, tatianau and aaronccasanova March 12, 2024 01:00

oksanashopify marked this pull request as draft March 12, 2024 16:47

oksanashopify requested a review from laurkim March 15, 2024 04:14

oksanashopify added the Feature request label Mar 15, 2024

oksanashopify marked this pull request as draft March 15, 2024 18:25

oksanashopify force-pushed the oa-indextable-row-dividers-configurable branch from 93b1513 to 2a0ba1d Compare March 15, 2024 18:28

oksanashopify added 2 commits March 15, 2024 21:32

fixed comments after rebase

869fd6e

oksanashopify force-pushed the oa-indextable-row-dividers-configurable branch from 2a0ba1d to e1db338 Compare March 15, 2024 21:55

Removed cell border styles

58148c1

oksanashopify marked this pull request as ready for review March 16, 2024 02:21

oksanashopify requested a review from aaronccasanova March 16, 2024 02:34

oksanashopify commented Mar 18, 2024

View reviewed changes

removed extra styles for borderless

804ca2c

oksanashopify requested a review from a team March 18, 2024 16:19

sam-b-rose reviewed Mar 20, 2024

View reviewed changes

chloerice reviewed Apr 15, 2024

View reviewed changes

aaronccasanova force-pushed the oa-indextable-row-dividers-configurable branch from 41ab59b to 804ca2c Compare April 19, 2024 21:52

oksanashopify requested a review from alex-page as a code owner April 19, 2024 21:52

github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024

aaronccasanova force-pushed the main branch from 9f6fc03 to b71eff5 Compare April 20, 2024 02:33

oksanashopify closed this Jul 10, 2024

	borderless && styles['TableRow-borderless'],
	borderColor && styles[variationName('borderColor', borderColor)],

[VF][Polaris] IndexTable row borders is configurable #11676

[VF][Polaris] IndexTable row borders is configurable #11676

Conversation

oksanashopify commented Feb 29, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

oksanashopify commented Feb 29, 2024

oksanashopify commented Feb 29, 2024

github-actions bot commented Feb 29, 2024

oksanashopify commented Mar 6, 2024

github-actions bot commented Mar 6, 2024

oksanashopify commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

tatianau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oksanashopify Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oksanashopify Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

oksanashopify Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

oksanashopify commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

oksanashopify commented Mar 15, 2024

oksanashopify commented Mar 15, 2024

oksanashopify commented Mar 15, 2024

github-actions bot commented Mar 15, 2024

oksanashopify commented Mar 16, 2024

github-actions bot commented Mar 16, 2024

oksanashopify Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

oksanashopify commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

sam-b-rose Mar 20, 2024 • edited by chloerice Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oksanashopify Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chloerice Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

chloerice Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translation-platform bot commented Apr 19, 2024

oksanashopify commented Jul 10, 2024

oksanashopify commented Feb 29, 2024 •

edited

Loading

tatianau left a comment •

edited

Loading

oksanashopify Mar 13, 2024 •

edited

Loading

oksanashopify Mar 12, 2024 •

edited

Loading

oksanashopify Mar 12, 2024 •

edited

Loading

oksanashopify Mar 18, 2024 •

edited

Loading

sam-b-rose Mar 20, 2024 •

edited by chloerice

Loading

oksanashopify Mar 26, 2024 •

edited

Loading

chloerice Apr 15, 2024 •

edited

Loading

chloerice Apr 15, 2024 •

edited

Loading