Skip to content
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

fix(table): change thead to sticky position with z-index #5887

Merged
merged 3 commits into from
Apr 20, 2020
Merged

fix(table): change thead to sticky position with z-index #5887

merged 3 commits into from
Apr 20, 2020

Conversation

virkt25
Copy link
Contributor

@virkt25 virkt25 commented Apr 19, 2020

This change makes thead stick to the top of the table container and ensures the elements in thead are visible as the table body is scrolled. Also allows for horizontal scrolling while honouring the sticky header.

Changelog

Changed

  • thead now uses position: sticky instead of position: fixed
  • thead has a z-index of 1

Testing / Reviewing

  • Create a table with a sticky header and enough data to cause scrolling (in both directions)

This change makes `thead` stick to the top of the table container and ensures the elements in `thead` are visible as the table body is scrolled.
@virkt25 virkt25 requested a review from a team as a code owner April 19, 2020 15:03
@ghost ghost requested review from asudoh and dakahn April 19, 2020 15:03
@netlify
Copy link

netlify bot commented Apr 19, 2020

Deploy preview for carbon-elements ready!

Built with commit aec5231

https://deploy-preview-5887--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 19, 2020

Deploy preview for carbon-components-react ready!

Built with commit aec5231

https://deploy-preview-5887--carbon-components-react.netlify.app

@tw15egan
Copy link
Member

tw15egan commented Apr 20, 2020

Getting a lot of top padding when enabling sticky header

Screen Shot 2020-04-20 at 11 01 14 AM

Also, as a heads up, doesn't seem like position: sticky is supported in IE11, so we'd better make sure we have some fallbacks there

Ref: https://caniuse.com/#search=position%3A%20sticky

@virkt25
Copy link
Contributor Author

virkt25 commented Apr 20, 2020

@tw15egan Fixed the padding issue. Carbon still supports IE11 😱 ? I don't think Microsoft even supports IE11 anymore. Carbon should consider just supporting evergreen browsers.

@tw15egan
Copy link
Member

@virkt25 at the moment we're still supporting IE11, but hopefully for not too much longer 😅

It doesn't look like the old sticky header functionality worked in IE11 anyways, so since it's an experimental feature I'm fine with keeping it like this 👍

@joshblack joshblack merged commit 53035b1 into carbon-design-system:master Apr 20, 2020
joshblack pushed a commit to joshblack/carbon that referenced this pull request Apr 23, 2020
…gn-system#5887)

* fix(table): change thead to sticky position with z-index

This change makes `thead` stick to the top of the table container and ensures the elements in `thead` are visible as the table body is scrolled.

* fix(data-table): remove padding as we switch to sticky position

Co-authored-by: TJ Egan <tw15egan@gmail.com>
joshblack pushed a commit to joshblack/carbon that referenced this pull request Apr 23, 2020
…gn-system#5887)

* fix(table): change thead to sticky position with z-index

This change makes `thead` stick to the top of the table container and ensures the elements in `thead` are visible as the table body is scrolled.

* fix(data-table): remove padding as we switch to sticky position

Co-authored-by: TJ Egan <tw15egan@gmail.com>
joshblack pushed a commit that referenced this pull request Apr 23, 2020
* fix(table): change thead to sticky position with z-index

This change makes `thead` stick to the top of the table container and ensures the elements in `thead` are visible as the table body is scrolled.

* fix(data-table): remove padding as we switch to sticky position

Co-authored-by: TJ Egan <tw15egan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants