-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat(table): added frozen header, frozen column, permanent scrollbars #2464
base: 18.4.0
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 82fe1dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -64,6 +68,34 @@ $density-relaxed-img-radius: 8px; | |||
padding: $density-relaxed-padding; | |||
} | |||
|
|||
.table--frozen-header thead { |
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 might not have full context, but I am see everywhere else using sticky
instead of frozen
even in figma playbook.
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.
We aren't necessarily bound to the vernacular of DS. We chart our own course for what makes sense for the web/users. Often times, DS actually reevaluates their terminology and follows suit as well.
I used frozen
instead of sticky
since that is par for the course on user-level understanding of table cells. We can rename if needed, but I'm not seeing the naming as an issue.
@@ -131,6 +163,11 @@ $density-relaxed-img-radius: 8px; | |||
text-align: center; | |||
width: 48px; | |||
} | |||
/* Not sure of downstream dev consumers' ability to utilize this. | |||
Convert to long-hand if problematic. */ | |||
.table--mode-selection tr:has(input:checked) :where(td, th) { |
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.
Did you include this by accident? Looks unrelated to the PR.
Also, :has
only has 91.66% global compatibility: https://caniuse.com/css-has
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's not an accident. This is to highlight the row on selection. Are you pulling down PRs locally when you review?
This doesn't impact function and is purely decorative. We do use :has
for similar types of progressive enhancements, for which current level of support is not really an issue.
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.
Got you. Just do not see the change in PR title and description.
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.
The markup was already done for selection. This is just a styling change; not all of those are specified granularly in PRs. No worries. Better safe than sorry. 😄
@@ -64,6 +68,34 @@ $density-relaxed-img-radius: 8px; | |||
padding: $density-relaxed-padding; | |||
} | |||
|
|||
.table--frozen-header thead { | |||
position: sticky; |
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.
Does this solution also work with dropdowns inside the cell?
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.
Good catch. Let me implement the full markup inside with open dropdowns and check. If it doesn't, I'll check for a css
solution, but we may end up having to do something via js
in ebay ui.
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.
@agliga , it works fine, at least with listbox. I have a feeling the issues the other team was having had to do with incorrect z-index
and lack of background-color
on <thead>
and all other non-data columns (outer wrapper).
Fixes #2457
Description
Added the next set of features to
table
:Notes
For the permanent scrollbars, I created a utility mixin to be used by a public utility class as well as intrinsically for
table
without the use of the utility class.Screenshots
Checklist