-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[IndexTable] Add sticky last cell #4150
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
|
🟢 This pull request modifies 5 files and might impact 1 other files. Details:All files potentially affected (total: 1)📄
|
e1a8503 to
25360e7
Compare
AndrewMusgrave
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.
2c6e842 to
d2611d6
Compare
25360e7 to
7c5b28e
Compare
size-limit report
|
| .TableHeading-last { | ||
| position: sticky; | ||
| right: 0; | ||
| } | ||
|
|
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 wasn't being referenced in any of the components.
9733aaa to
cc90df7
Compare
| $index-table-stacking-order: ( | ||
| cell: 1, | ||
| sticky-cell: 30, | ||
| sticky-cell: 31, |
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.
A focused TextField has a z-index of 30, so this one was bumped up to avoid weirdness when using them together. 31 is not used elsewhere.
Before:
z-before.mov
After:
z-after.mov
cc90df7 to
2597850
Compare
| @include breakpoint-after($breakpoint-small) { | ||
| box-shadow: 0 rem(-1px) 0 0 var(--p-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 because the "border" appears on the TableRow
| toggle: toggleHasMoreLeftColumns, | ||
| } = useToggle(false); | ||
|
|
||
| const onboardingScrollButtons = useRef(false); |
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 ref isn't being used by anything in this file and never gets toggled to true 🤔
| if (!canScrollRight) { | ||
| onboardingScrollButtons.current = false; | ||
| } |
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.
| {title: 'Name'}, | ||
| {title: 'Location'}, | ||
| {title: 'Order count'}, | ||
| {title: 'Amount spent', hidden: false}, |
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.
Added the optional hidden here to bring attention that it could be set to true as well.
@AndrewMusgrave I re-worked this PR to take your two points into consideration! The PR's description has been updated. |
2597850 to
3ec5bff
Compare
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.
Tested in all supported browsers and on mobile and this works great 🙌🏽 Thanks for contributing @pamelahicks! 💟
AndrewMusgrave
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.
Left a minor comment. LGTM, nice work ✅ !!
fbfebce to
4da6801
Compare

WHY are these changes introduced?
Resolves #4149
Resolves #4247
WHAT is this pull request doing?
Previous attempt
This PR adds the
lastprop which toggles the sticky class. The column will only be sticky above$breakpoint-small.Styleguide:
This PR:
lastColumnStickyprop to the IndexTable which toggles the sticky class. The column will only be sticky above$breakpoint-small.hidden.box-shadows which were no longer working on sticky-positioned cells in Chrome.z-indexfrom 30 to 31 to avoid collisions with focused TextFields (which arez-index: 30).Approach
Prop
The previous attempt (collapsed above) simply added a
lastprop to a cell to make it stick, but I decided to apply a proplastColumnStickyto the table instead. This is to allow easy access to the prop for the purposes of adding it to the last heading, in response to this comment.Cell borders
The styles were refactored to re-add the missing cell "borders" on the sticky cells. I opted to go with
filter: drop-shadowbecause like box-shadow, it does not add height or width to the box, and has acceptable bowser support of 97.08% (source), which is even better than the support forposition: sticky.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:Style guide (Chrome):
chrome.2.55.03.PM.mov
Chrome with the last heading hidden:
chrome-hidden.mov
Firefox:
Firefox.mov
Safari:
safari.mov
iOS Safari: https://user-images.githubusercontent.com/13036921/124321724-3221b780-db4c-11eb-871f-a893ee93868a.jpeg
Condensed (Chrome):

🎩 checklist
README.mdwith documentation changes