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

Sticky table headers on universal listings #11798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yhaooo11
Copy link

Makes table headers sticky for tables with the full-width class.

This should implement the enhancement for the issue #11705

image

image

Copy link

squash-labs bot commented Mar 26, 2024

Manage this branch in Squash

Test this branch here: https://yhaooo11enhancement11705-stick-o07cy.squash.io

@lb- lb- self-requested a review April 5, 2024 09:15
@lb-
Copy link
Member

lb- commented Apr 23, 2024

@thibaudcolas or @laymonage - when possible, it might be good for you to look at this.

I will try to do a review in the coming week or so but pinging as you may have thoughts.

Thank you @yhaooo11 for preparing this PR

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few suggestions from a code variable usage perspective.

Comment on lines +110 to +119

thead {
position: sticky;
top: 140px;
@include media-breakpoint-up(sm) {
top: 70px;
}
background-color: theme('colors.surface-page');
z-index: 3;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not run this code locally yet but in general we should avoid hard-coded values and instead use variables.

We use this 70px in lots of places, not sure on the best location for this variable but maybe we should add to client/tailwind.config.js.

      addBase({
        /** Support for web components */
        ':root, :host': {
          '--w-font-sans': fontFamily.sans.join(', '),
          '--w-font-mono': fontFamily.mono.join(', '),
          '--w-density-factor': '1',
++          '--w-header-height': '4.375rem', // 70px

Then we can update as below.

Suggested change
thead {
position: sticky;
top: 140px;
@include media-breakpoint-up(sm) {
top: 70px;
}
background-color: theme('colors.surface-page');
z-index: 3;
}
thead {
position: sticky;
top: calc(var(--w-header-height) * 2);
@include media-breakpoint-up(sm) {
top: var(--w-header-height);
}
background-color: theme('colors.surface-page');
z-index: calc(theme('zIndex.header') - 20); // not sure on this but start here and see what works
}

@lb- lb- added status:Needs Review component:Design system Including the pattern library (Storybook) component:Universal listings Including page listings, model listings and filtering. labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Design system Including the pattern library (Storybook) component:Universal listings Including page listings, model listings and filtering. status:Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants