Skip to content

[Test] Test sticky positioning on thead#5288

Closed
amyleadem wants to merge 16 commits intodevelopfrom
al-sticky-table-header
Closed

[Test] Test sticky positioning on thead#5288
amyleadem wants to merge 16 commits intodevelopfrom
al-sticky-table-header

Conversation

@amyleadem
Copy link
Contributor

@amyleadem amyleadem commented May 16, 2023

Summary

This PR is built as a test for adding sticky positioning to table headers. This PR:

  • Provides a prototype for testing position: sticky on the thead element instead of the th cells.
  • Adds Storybook controls for testing stickyheader, scrollable, and multiple header rows on all table variants for improved testing.

⚠️ This PR is for testing purposes only and should not be merged. The code is built as a prototype for testing and should be refined if we want to bring anything into production.

Related pull requests

This PR is based on and is a test for PR #5074

Preview link

Preview link: Table component

@amyleadem amyleadem changed the title [Test] Sticky header for testing [Test] Test sticky positioning on thead May 17, 2023
@amyleadem
Copy link
Contributor Author

amyleadem commented May 24, 2023

@mejiaj @mahoneycm
I put together this PR for testing the sticky table headers. I've added my test notes below with some suggestions for improvement.

Note: I did a bit more digging after our conversation about this last week, looking mostly into modern recommendations for styling tables, but did not find anything that made me want to shift my approach. Happy to hear if anyone has any recommendations for improvement.

There are a few items that need team input and I've tagged items those items with the ⚠️ icon below. Will you let me know if you have thoughts on any of these (or other) items?

I have tested the following items:

  • Works with default variant
  • Works as expected on headers with multiple rows
    • After doing a bit of digging, it looks like we actually can add position:sticky to the thead element instead of th. It looks like support for sticky positioning on thead is now available in most browsers (Reference: caniuse - position sticky). Additionally, preliminary tests in USWDS (available in this test PR) show that applying sticky to thead works in the most commonly-used browsers.
    • Costs of applying sticky to thead:
      • Need to style borders in a different way: In Chrome, the background content is visible behind where the cell borders should be when using this technique. We can work around this with border-collapse: separate, but it would require reworking how our borders are applied to table cells in order to maintain current appearance.
      • Does not have universal support: Some minor browsers do not support sticky positioning on thead, but they appear to fall below the 2% threshold. (Reference: caniuse - position sticky)
      • ⚠️ Can you test the preview link and confirm that there are no issues with applying sticky to thead?
  • Works with borderless variant
    • The table header background color changes when stickyheader is turned on. By default the background is set to transparent but when stickyheader is turned on, it sets background color to $theme-table-header-background-color.
    • ⚠️ We should consider creating a $theme-table-background-color setting that can be used to also set the header background color. Alternatively, we could create a $theme-table-borderless-background-color specifically for this role. This PR demonstrates $theme-table-background-color. Thoughts?
  • Works with striped variant
  • Works with sortable variant
    • Sticky headers do not currently work on the sortable variant because .usa-table th[data-sortable] overrides sticky positioning with position: relative. To resolve this, we could either update the styles so that position:sticky is applied when sticky headers are turned on OR apply sticky positioning to the thead (refer to the notes on multiple rows above for details)
    • Additionally, it would be helpful to be able to easily test sticky headers on this variant as well. Here are a couple of items that would help with that:
      • It would be great if we had the {{stickyheader ? 'usa-table--sticky-header' : ''}} check that you added in usa-table.twig in usa-table--sortable.twig
      • Additionally, could you add a JSON file for sortable tables with entries for stickyheader and scrollable?
      • Right now usa-table--sortable.twig has the usa-table-container--scrollable wrapper hard-coded in. Could you make this optional like we do in usa-table.twig
    • ⚠️ No decision is needed on this item if we apply sticky to thead
  • Works as expected on scrollable and stacked variants
    • Note: I confirmed that it is a known problem that sticky positioning does not work with overflow:hidden ancestors. (Related CSS working group issue for context: [css-position-3] Making a stickypos in a scroller also see the viewport edges w3c/csswg-drafts#8286) I could not find a semantic, non-JS workaround.
      • ⚠️ Is anyone else able to find a solution/workaround they like for this?
      • ⚠️ I don't feel like this needs to be a blocker at this time. Unless there is a clear solution now, it feels like this could be a possible follow-on enhancement we can approach at a later time.
    • ⚠️ Does it make sense to use sticky positioning on the stacked variant? I'm thinking not but curious about other opinions.
  • Test on Chrome, Firefox, Safari, iOS
  • Test at varying breakpoints

Other considerations

⚠️ We could add a setting for a customizable top offset: This PR demonstrates a $theme-table-sticky-top-offset setting that allows users to customize the top offset in case their site has additional elements that stick to the top (like the header on uswds-site). It is not written with units at this time because I thought the number might need to be a very specific value. I Any objections?

@amyleadem amyleadem requested review from mahoneycm, mejiaj and scottqueen-bixal and removed request for scottqueen-bixal May 24, 2023 14:49
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Thanks for your work to make this easily viewable! Tests are working great for me and I like implementing sticky on thead. That seems like the most logical approach to me.

  • Works on default
  • No issues on sticky thead across variants
  • Works with borderless variant
    • like the current iteration using $theme-table-background-color
    • What’s the best way for users to change the table header color with this method?
      • If we created a $theme-table-borderless-background-color and set it to equal $theme-table-background-color, users could override to use $theme-table-header-background-color if they want to change borderless header background color.
  • Works on striped variant
  • Works on sortable as expected with sticky on thead
  • Cannot find solution for scrollable tables

@mejiaj
Copy link
Contributor

mejiaj commented May 25, 2023

Testing sticky

⚠️ Can you test the preview link and confirm that there are no issues with applying sticky to thead?

Can we clarify at the end of this note to use storybook controls? Example: …applying sticky to thead via StorybookJS controls

I tested without issues with the following Mac browsers:

  • Firefox 114
  • Chromium 113
  • Safari 16.4

Table color setting

⚠️ We should consider creating a $theme-table-background-color setting that can be used to also set the header background color. Alternatively, we could create a $theme-table-borderless-background-color specifically for this role. This PR demonstrates $theme-table-background-color. Thoughts?

We currently have the following settings:

  • $theme-table-header-background-color
  • $theme-table-stripe-background-color
  • $theme-table-sorted-header-background-color
  • $theme-table-sorted-background-color
  • $theme-table-sorted-stripe-background-color
  1. Would this new setting be for the entire table or just body?
  2. Do you see any conflicts with the already existing settings?

Table sortable

⚠️ No decision is needed on this item if we apply sticky to thead

Note: we should consider templatizing some of our tables, so they're not hardcoded and can be re-used. ⚠️ Might be a couple of days to get the right format for data and markup.


Scrollable and stacked variants

⚠️ Is anyone else able to find a solution/workaround they like for this?

We'd need to create a SPIKE issue to research this.

⚠️ I don't feel like this needs to be a blocker at this time. Unless there is a clear solution now, it feels like this could be a possible follow-on enhancement we can approach at a later time.

I'd be OK with handling this in guidance, for now. If we're clear that sticky header is made for certain variants.

⚠️ Does it make sense to use sticky positioning on the stacked variant? I'm thinking not but curious about other opinions.

My immediate thought is "no" for this initial attempt. We can add it as an enhancement later on if necessary.


Other

⚠️ We could add a setting for a customizable top offset: This PR demonstrates a $theme-table-sticky-top-offset setting that allows users to customize the top offset in case their site has additional elements that stick to the top (like the header on uswds-site). It is not written with units at this time because I thought the number might need to be a very specific value. I Any objections?

The headers can be any height based on content, so makes sense if it doesn't work in units.

Copy link
Contributor Author

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Accidental comment

@amyleadem
Copy link
Contributor Author

Closing because we no longer need this test PR

@amyleadem amyleadem closed this Aug 2, 2023
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.

4 participants