-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[DataTable] Better prop compatibility and fixing no-wrap issue.
#7043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
size-limit report 📦
|
… or `condensed` is `true`.
| .FixedFirstColumn { | ||
| position: absolute; | ||
| background: var(--p-surface); | ||
| background: inherit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to assume that the table is --p-surface. However that might not be the case. This ensures it always is whatever the defined DataTable background is set to.
| &:last-of-type { | ||
| border-right: var(--p-border-divider); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed since now any cell that needs to be separated simply has the Cell-Seperate class.
| if (table && scrollContainer) { | ||
| condensed = table.scrollWidth > scrollContainer.clientWidth; | ||
| // safari sometimes incorrectly sets the scrollwidth too large by 1px | ||
| condensed = table.scrollWidth > scrollContainer.clientWidth + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| maxWidth: `${ | ||
| columnVisibilityData[fixedFirstColumns - 1]?.rightEdge | ||
| }px`, | ||
| width: `${columnVisibilityData[fixedFirstColumns - 1]?.rightEdge}px`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused issues with things lining up correctly. We always want the fixed first columns table to match the correct width of the underlying table. maxWidth gives no benefit but instead leaves room for error.
| } else { | ||
| this.tableHeadings[index] = ref; | ||
| this.tableHeadingWidths[index] = ref.getBoundingClientRect().width; | ||
| this.tableHeadingWidths[index] = ref.clientWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically getBoundingClientRect().width gives a more accurate number, but the rounding ends up being inconsistent between the headings and the headings in the sticky header table. Overall clientWidth gives more consistent results as it provides a integer vs a float.
| const {stickyHeader} = this.props; | ||
|
|
||
| if (condensed && table && scrollContainer && dataTable) { | ||
| if ((stickyHeader || condensed) && table && scrollContainer && dataTable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stickyHeader also needs the measurements to correctly apply the widths to it's headings.
| borderRight: | ||
| headingIndex === fixedFirstColumns - 1 && fixedCellVisible | ||
| ? 'var(--p-border-divider)' | ||
| : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was impossible to get the borders to be in the exact same position in the different contexts this way. I had to introduce lastFixedFirstColumn to be able to use CSS ::after pseudo-element which gave more reliable results.
| setRef(ref); | ||
| }} | ||
| style={{...minWidthStyles}} | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just re-ordered the above props to match the order.
| </TruncatedText> | ||
| ) : ( | ||
| content | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we only truncate if we actually have the truncate prop. @brianshen1990, this is what fixes the issue you were encountering.
brianshen1990
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From bulk-account-inviter's prospective, this PR looks great!
|
/snapit |
|
🫰✨ Thanks @nickpresta! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20220829184636yarn add @shopify/polaris@0.0.0-snapshot-release-20220829184636 |
|
Thanks @nickpresta for creating the snapshot. I should have done this. Here is the spin instance for other reviewers which contains the snapshot: https://shop1.shopify.data-table-enhancements.philipp-schofer.us.spin.dev/admin/ |
nickpresta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this in our Cohort Analysis branch and things look good. Thanks!
chloerice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌🏽 ![]()
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-icons@6.0.0 ### Major Changes - [#7012](#7012) [`bd00ef4ed`](bd00ef4) Thanks [@leileu](https://github.com/leileu)! - Adding Metafields icon to polaris ### Minor Changes - [#7013](#7013) [`1e0645f33`](1e0645f) Thanks [@jpmarra](https://github.com/jpmarra)! - Added IdentityCard icon - [#7028](#7028) [`635bcfeb7`](635bcfe) Thanks [@joelzwarrington](https://github.com/joelzwarrington)! - add vertical viewport icon variations ## @shopify/polaris@10.1.0 ### Minor Changes - [#6976](#6976) [`ae7345f0c`](ae7345f) Thanks [@tylernoseworthy](https://github.com/tylernoseworthy)! - - Added a `fixedFirstColumns` prop to `DataTable` so that multiple columns can be fixed - Deprecated the `DataTable` `fixedFirstColumn` prop - [#7043](#7043) [`60086a61f`](60086a6) Thanks [@philschoefer](https://github.com/philschoefer)! - Updates to `DataTable` - Fixed `DataTable` cell content not wrapping when the `truncate` prop is `false` - Added support for setting the `DataTable` `truncate` prop without having to set the `fixedFirstColumns` prop ### Patch Changes - [#7022](#7022) [`716956df6`](716956d) Thanks [@QuintonC](https://github.com/QuintonC)! - Fixed visual bug for avatar shape prop - [#7038](#7038) [`d1a33d8b0`](d1a33d8) Thanks [@kyledurand](https://github.com/kyledurand)! - Applied default background color to image avatar - [#6993](#6993) [`fa840e4a9`](fa840e4) Thanks [@kyledurand](https://github.com/kyledurand)! - Removed deprecation from Layout.AnnotatedSection - [#7003](#7003) [`2b5f7d0fc`](2b5f7d0) Thanks [@mrcthms](https://github.com/mrcthms)! - Fix visual bug for sortable, selectable index table headings - Updated dependencies \[[`bd00ef4ed`](bd00ef4), [`1e0645f33`](1e0645f), [`635bcfeb7`](635bcfe)]: - @shopify/polaris-icons@6.0.0 ## polaris.shopify.com@0.14.0 ### Minor Changes - [#7018](#7018) [`a8087a358`](a8087a3) Thanks [@alex-page](https://github.com/alex-page)! - Generate assets once with a seperate script - [#6934](#6934) [`793e26f4d`](793e26f) Thanks [@alex-page](https://github.com/alex-page)! - Use @shopify/polaris-icons instead of /icons and copy-icons - [#7049](#7049) [`7a548a00a`](7a548a0) Thanks [@alex-page](https://github.com/alex-page)! - Move search to the server side ### Patch Changes - [#7015](#7015) [`e612cbccb`](e612cbc) Thanks [@lgriffee](https://github.com/lgriffee)! - Update navigation column in breakpoints table for MD breakpoint - [#7027](#7027) [`a805116a6`](a805116) Thanks [@sarahill](https://github.com/sarahill)! - Updated design guidance for typography - Updated dependencies \[[`716956df6`](716956d), [`ae7345f0c`](ae7345f), [`60086a61f`](60086a6), [`bd00ef4ed`](bd00ef4), [`d1a33d8b0`](d1a33d8), [`fa840e4a9`](fa840e4), [`2b5f7d0fc`](2b5f7d0), [`1e0645f33`](1e0645f), [`635bcfeb7`](635bcfe)]: - @shopify/polaris@10.1.0 - @shopify/polaris-icons@6.0.0 ## polaris-for-figma@0.0.11 ### Patch Changes - Updated dependencies \[[`716956df6`](716956d), [`ae7345f0c`](ae7345f), [`60086a61f`](60086a6), [`d1a33d8b0`](d1a33d8), [`fa840e4a9`](fa840e4), [`2b5f7d0fc`](2b5f7d0)]: - @shopify/polaris@10.1.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

WHY are these changes introduced?
@brianshen1990 was integrating the new
DataTablein theBulkAccountInviterapp and noticed that regular cells were never wrapping in multiple lines, pushing the table width beyond the available space. This triggered the condensed nav. This PR fixes this by defaulting to cells wrapping unless thetruncateprop is specifically set.This was caused by this PR which wrapped all cells in a
TruncatedTextcomponent which would automatically add tooltips in case content doesn't have enough space. This was a bit too heavy handed. After this PRTruncatedTextis only applied if thetruncateprop is specifically set.I also ensured the fixed first column feature will work properly no matter whether
truncatedis set or not. This means you can now have fixed first columns with text wrapping and with text being truncated depending on what the consumer prefers.Lastly, there seemed to be a bug in the calculation of headers when
fixedFirstColumsis set to a value larger than 1. This was because currently we're only calculatingcolumnVisibilityDatawhencondensedwastrue. This means no heading widths are available to properly calculate the absolute positioning of fixed headings in the sticky header unless the viewport width was reduced to triggercondensedto betrue. After this PRcolumnVisibilityDatawill be calculated when eitherstickyHeadingistrueor the table iscondensed.Screenshot of wrapping issue
Video of heading with calculation issue
WHAT is this pull request doing?
GIF of sticky heading width calculated correctly
GIF of truncation with and without fixed first column
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Spin-URL: (
shopify/web) https://shop1.shopify.data-table-enhancements.philipp-schofer.us.spin.dev/admin/truncateon/offfixedFirstColumns- 1,2,3 or 0stickyHeaderon/offCopy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes