[IndexTable] consolidate se23 styles and logic #10340

gwyneplaine · 2023-09-06T00:07:26Z

WHY are these changes introduced?

Fixes #9939

WHAT is this pull request doing?

IndexTable consolidate se23 logic
IndexTable consolidate se23 styles

How to 🎩

🎩 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

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

laurkim

Nice clean up! 🚀 🙌

Small things I noticed while tophatting below otherwise everything looked good across different breakpoints. Some of the IndexFilters issues I can create a separate issue for though, if we don't want to conflate the scope of this PR.

content jumps to keep table headers visible for BulkActions unlike prod (stories with BulkActions)
spacing around IndexFilters is a bit off when comparing to prod — 8px top/bottom on prod as opposed to 6px on this branch (With Filtering story)

bg color on hover for table row with zebra striping is off (With Zebra Striping story)
When BulkActions are visible, header wrapper is missing right side padding for xs screen (With Bulk Actions and Selections Across Pages story)
prod:

this branch:

With Filtering example has larger height on filters and extra padding around actions for xs screen

IndexFilters has larger height than prod on xs screen (Small Screen With All Of Its Elements story)

gwyneplaine · 2023-09-11T04:55:44Z

Thnx for the review @laurkim

content jumps to keep table headers visible for BulkActions unlike prod (stories with BulkActions)

Fixed!

spacing around IndexFilters is a bit off when comparing to prod — 8px top/bottom on prod as opposed to 6px on this branch (With Filtering story)

Having a hard time replicating this, looks to be 6px vertical padding in prod as well, let me know if I'm missing something 🙏

bg color on hover for table row with zebra striping is off (With Zebra Striping story)

This looks to be a specificity bug on prod and not intended behaviour

When BulkActions are visible, header wrapper is missing right side padding for xs screen (With Bulk Actions and Selections Across Pages story)
prod:

Fixed!

With Filtering example has larger height on filters and extra padding around actions for xs screen

Small fix to IndexFilter which I've added to this PR 👍

IndexFilters has larger height than prod on xs screen (Small Screen With All Of Its Elements story)

Had a hard time replicating this one, seeing the same height on prod and on local 57px

, let me know what I'm missing. Since this is IndexFilter specific though, may fix this in a separate PR.

polaris-react/src/components/IndexFilters/IndexFilters.scss

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

jesstelford · 2023-09-11T06:08:11Z

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

 }

 .TableCell-first + .TableCell {
  // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
  left: var(--pc-checkbox-offset);
-  padding-left: 0;


Where'd this one go? I can't see it being set anywhere.

I think it's there to ensure there's no accidental extra padding between the sticky first column and the second column when horizontal scrolling.

I removed this, because it was causing unwanted layout shift on sticky
Tested this quite a bit, whatever was causing the extra padding no longer exists so I removed this to resolve the prior layout shift bug.

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

polaris-react/src/components/IndexFilters/IndexFilters.scss

laurkim

tysm for resolving flagged issues, looks good now when tophatting locally 🙌 💯

Thnx for the review @laurkim

spacing around IndexFilters is a bit off when comparing to prod — 8px top/bottom on prod as opposed to 6px on this branch (With Filtering story)

Having a hard time replicating this, looks to be 6px vertical padding in prod as well, let me know if I'm missing something 🙏

IndexFilters has larger height than prod on xs screen (Small Screen With All Of Its Elements story)

Had a hard time replicating this one, seeing the same height on prod and on local 57px

, let me know what I'm missing. Since this is IndexFilter specific though, may fix this in a separate PR.

For IndexFilters padding, I'm still seeing 8px all around on prod storybook for main and next branches 🤔 Same with height issue on IndexFilters for Small Screen With All Of Its Elements story where if you change the breakpoint to xs screen, the height on IndexFiltersWrapper is 49px on prod but 57px for local. I think these 2 remaining issues aren't a blocker for this PR though and can be resolved in a separate one since they seem to be IndexFilters specific 👍

Fixes #9939 - `IndexTable` consolidate se23 logic - `IndexTable` consolidate se23 styles - [Storybook](https://5d559397bae39100201eedc1-jsgyqvgaeb.chromatic.com/?path=/story/all-components-indextable--with-all-of-its-elements) - [Production storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-indextable--with-all-of-its-elements&globals=polarisSummerEditions2023:true) - [ ] 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) - [ ] 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

### WHY are these changes introduced? Fixes Shopify#9939 ### WHAT is this pull request doing? - `IndexTable` consolidate se23 logic - `IndexTable` consolidate se23 styles ### How to 🎩 - [Storybook](https://5d559397bae39100201eedc1-jsgyqvgaeb.chromatic.com/?path=/story/all-components-indextable--with-all-of-its-elements) - [Production storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-indextable--with-all-of-its-elements&globals=polarisSummerEditions2023:true) ### 🎩 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) - [ ] 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

gwyneplaine linked an issue Sep 6, 2023 that may be closed by this pull request

[IndexTable] Consolidate se23 logic and styles #9939

Closed

gwyneplaine changed the title ~~[IndexTable] consolidate se23 styles~~ [IndexTable] consolidate se23 styles and logic Sep 6, 2023

gwyneplaine force-pushed the v12/9939 branch 2 times, most recently from 8b65f8d to 42c41bd Compare September 6, 2023 01:26

gwyneplaine commented Sep 6, 2023

View reviewed changes

polaris-react/src/components/IndexTable/IndexTable.scss Outdated Show resolved Hide resolved

gwyneplaine force-pushed the v12/9939 branch 2 times, most recently from 6e291a9 to b70302c Compare September 6, 2023 02:00

gwyneplaine marked this pull request as ready for review September 6, 2023 02:03

gwyneplaine requested review from jesstelford, chloerice, sophschneider, sam-b-rose and laurkim September 6, 2023 03:43

laurkim reviewed Sep 7, 2023

View reviewed changes

gwyneplaine force-pushed the v12/9939 branch from b70302c to 3ccc3d6 Compare September 11, 2023 00:32

gwyneplaine requested a review from laurkim September 11, 2023 04:55

gwyneplaine force-pushed the v12/9939 branch from 3ccc3d6 to e9e0d60 Compare September 11, 2023 04:59

jesstelford requested changes Sep 11, 2023

View reviewed changes

gwyneplaine requested a review from jesstelford September 11, 2023 06:56

gwyneplaine force-pushed the v12/9939 branch from 7ca1f61 to 5f1e9ac Compare September 11, 2023 07:13

jesstelford requested changes Sep 11, 2023

View reviewed changes

polaris-react/src/components/IndexFilters/IndexFilters.scss Outdated Show resolved Hide resolved

laurkim approved these changes Sep 11, 2023

View reviewed changes

gwyneplaine added 2 commits September 12, 2023 09:34

IndexTable consolidate se23 logic

126f8b3

IndexTable consolidate se23 styles

0700c14

gwyneplaine force-pushed the v12/9939 branch from 5f1e9ac to 0700c14 Compare September 11, 2023 23:39

gwyneplaine requested a review from jesstelford September 11, 2023 23:40

jesstelford approved these changes Sep 12, 2023

View reviewed changes

gwyneplaine merged commit 50dd08b into next Sep 12, 2023
12 checks passed

gwyneplaine deleted the v12/9939 branch September 12, 2023 00:25

sam-b-rose mentioned this pull request Sep 19, 2023

Remove nav level rule #10568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IndexTable] consolidate se23 styles and logic #10340

[IndexTable] consolidate se23 styles and logic #10340

gwyneplaine commented Sep 6, 2023 •

edited

Loading

laurkim left a comment •

edited

Loading

gwyneplaine commented Sep 11, 2023

jesstelford Sep 11, 2023

gwyneplaine Sep 11, 2023

laurkim left a comment

[IndexTable] consolidate se23 styles and logic #10340

[IndexTable] consolidate se23 styles and logic #10340

Conversation

gwyneplaine commented Sep 6, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

laurkim left a comment • edited Loading

Choose a reason for hiding this comment

gwyneplaine commented Sep 11, 2023

jesstelford Sep 11, 2023

Choose a reason for hiding this comment

gwyneplaine Sep 11, 2023

Choose a reason for hiding this comment

laurkim left a comment

Choose a reason for hiding this comment

gwyneplaine commented Sep 6, 2023 •

edited

Loading

laurkim left a comment •

edited

Loading