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

feat(bcd): make BCD table's header sticky #8541

Closed
wants to merge 15 commits into from

Conversation

yarusome
Copy link
Contributor

Summary

Fixes #5417.

Problem

This PR solves the problem that lengthy BCD tables are less easy to read when the headers are scrolled out of the viewport.

Solution

This PR makes the table header sticky.


Screenshots

Before

before

After

after

@caugner caugner requested a review from LeoMcA March 31, 2023 12:42
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

This works great on desktop, but removing the overflow: auto on .table-container exposes a couple of bugs:

first, we've got some sort of minimum sizing issue with the table as a whole when approaching the screen-lg breakpoint:

Screen Shot 2023-04-03 at 15 13 24

second, once we're in the screen-lg breakpoint we relied on the prior overflow: auto behaviour to be able to scroll the overflowing table around, now the whole page scrolls and it overflows in a not very nice way:

image

since we can't keep that overflow: auto due to it resetting the scrolling context for the sticky element, I think what we'll have to do is:

  • below screen-sm, keep the behaviour as it is
  • from screen-sm to below screen-xl: set the table to a max height of 100vh - the top navigation and width of the content section, set overflow scroll, and make the header sticky to the scroll context of the table
  • screen-xl and above: keep the behaviour as exists now in this pr (with a fix for the first problem above when approaching the "lg limit"), no scrolling in the table itself - the whole page scrolls and the header is sticky to the page itself

@yarusome
Copy link
Contributor Author

yarusome commented Apr 4, 2023

@LeoMcA The first problem occurs even for screen-xl and above since there's one interface with a name as ridiculously long as contentvisibilityautostatechange. When 1200 <= window.width <= 1300 on the Element page, the BCD table still needs a horizontal scrollbar to show completely, so it seems that screen-xl and above needs to set max height as well.

To me, the ultimate source of problems is the code blocks in the BCD tables. Is it feasible to add overflow-wrap: anywhere or even word-break: break-all on these blocks instead?

@yarusome yarusome marked this pull request as draft April 5, 2023 12:22
@yarusome
Copy link
Contributor Author

yarusome commented Apr 6, 2023

@LeoMcA I changed the scope of this PR down to only for screen-md and above, and also made a few other changes to make both screen-md and scree-xlwork.

Screenshot on the Window page with window.innerWidth = 770:

width@770

A weird phenomenon is that position: sticky seems to behave badly (only) on the Element page.

Now I'm not so sure if making BCD tables sticky is a good idea after all.

@yarusome yarusome marked this pull request as ready for review April 6, 2023 06:09
@yarusome yarusome changed the title feat(client): make BCD table's header sticky feat(bcd): make BCD table's header sticky Apr 16, 2023
@yarusome
Copy link
Contributor Author

I finally managed to make the header sticky and its border not drifting away when scrolling. However:

  1. The outermost wrapper for BCD tables, i.e. figure.table-container, needs adding an extra class bc-table-container, but I didn't figure out how to manipulate it among those .tsx files.
  2. I haven't set min-height on .bc-table-container since I'm not sure what value is appropriate, but there should be one for cases where innerHeight is too small.
  3. The case between screen-sm and screen-md is still left as it is because I couldn't make a vertical scrollbar appear even if there's nothing like overflow-y: hidden set.

In the screenshots below, --border-primary and --elem-radius are altered for clearer demostration.

innerWidth = 900px:

screenshot@900px

innerWidth = 1160px:

screenshot@1160px

innerWidth = 1280px:

screenshot@1280px

innerWidth = 1350px:

screenshot@1350px

@yarusome yarusome requested a review from LeoMcA April 16, 2023 07:59
@LeoMcA
Copy link
Member

LeoMcA commented May 12, 2023

@yarusome thanks for all your work here, and apologies for the long delay - I was out sick, then had some rapidly approaching deadlines

It's a shame that position sticky can't "skip" a scrolling context - if it could I think this would be purely an enhancement, and easy to merge - however, since it isn't, and we change the behaviour of the BCD tables quite a bit, I'll need to discuss this one with the rest of the team.

The concern I have with making the BCD table itself scrollable is it's not immediately obvious that there's more to see when scrolling, and it's easy to skip over information when then scrolling the page.

The concern I have with forcing word breaking on the code blocks is it's pretty ugly breaking mid-word - this is something I've tackled before in the collections feature by adding <wbr> (or, actually, the unicode equivalent character) to break on .attributes and camelCase strings appropriately, but the example of contentvisibilityautostatechange is, unfortunately, neither so we'd have to define this manually somewhere. This is something we may want to tackle more broadly - across titles and sidebars, for instance - so again, I'll discuss this with the team.

The outermost wrapper for BCD tables, i.e. figure.table-container, needs adding an extra class bc-table-container, but I didn't figure out how to manipulate it among those .tsx files.

I'll push a commit adding the bc-table-container class to the appropriate element for completeness' sake, but don't feel pressured by that into continuing to work on it (unless, of course, you're relishing the css challenge and want to solve the seemingly unsolvable!) - I'll be back in touch after discussing with the team

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.

Suggestion: Make compatibility table header sticky
2 participants