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/6475 Table of Contents overflowing side of window and global footer #140

Merged
merged 8 commits into from Oct 11, 2022

Conversation

adamwoodnz
Copy link
Contributor

@adamwoodnz adamwoodnz commented Sep 14, 2022

Fixes https://meta.trac.wordpress.org/ticket/6475

2 issues are fixed by this PR:

  1. If a user has scrollbars set to always be displayed the ToC can overflow the screen on the right at certain sizes (~1440px wide)
  2. On short screens the ToC can cover the global footer when scrolled right to the bottom

For 2 a script has been added which detects if the ToC is overlapping the footer on scroll and resize, and if so shifts it up above. lodash.throttle has been used to throttle the events, as opposed to requestAnimationFrame, as it provides trailing event support. This proved necessary in many browsers where there is a delay in restoring the scroll position after refreshing, and rAF throttling blocked these delayed events, resulting in the ToC not being repositioned on refresh when scrolled to the bottom of the page.

As there is no build process for JS in this repo I've opted for es6 local import, but we could introduce a build instead. I think browser support is good enough for this, based on our support policy and https://caniuse.com/es6-module-dynamic-import

Screenshots

Horizontal overflow

Before After
Screen Shot 2022-10-05 at 11 51 06 AM Screen Shot 2022-10-05 at 11 51 51 AM

Footer overlap

Before
Screen Shot 2022-10-05 at 11 54 02 AM

After
Kapture 2022-10-05 at 11 55 59

How to test

  1. Set scrollbars to always be displayed (MacOS: System Prefs > General > Show scroll bars: 'Always')
  2. Open one of the pages with a ToC (eg. http://localhost:8888/reference/classes/wp_error/get_error_code/) and resize your browser window to 1440px x 705px
  3. Ensure the ToC does not overflow the side of the window
  4. Scroll to the bottom of the page, both manually and by using the links in the ToC
  5. Ensure the ToC does not cover the footer
  6. Refresh the page and ensure the position is updated when scroll position is restored.
  7. Try combinations of scrolling up and down and resizing the window. The ToC should reposition itself.
    NOTE: at smaller sizes the ToC will not be fixed outside the content, and the layout should not be effected by the new changes.
  8. Try other pages like Rest API or Coding Standards.
    NOTE: locally the lefthand sidebar may not be displayed on these pages (it's not for me). To mimic the actual behaviour in prod you can comment out the return on this line: https://github.com/WordPress/wporg-developer/blob/trunk/source/wp-content/themes/wporg-developer/sidebar-handbook.php#L9

top: $single-handbook-toc-position-top;
max-height: calc(90vh - #{ $single-handbook-toc-position-top });
}
}
}

@media (min-width: 877px) {
// Pages which are not code reference and without a sidebar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was required for local dev, but in production all pages have a sidebar on the left so this code is redundant

@adamwoodnz adamwoodnz marked this pull request as ready for review October 4, 2022 23:17
@adamwoodnz adamwoodnz changed the title Fix/6475 toc overflow Fix/6475 Table of Contents overflowing side of window and global footer Oct 4, 2022
In several browsers there is a delay before the scroll position is restored.
This results in the ToC not being repositioned if the page is refreshed when scrolled to the bottom.
Lodash Throttle provides support for `trailing` events to always be fired, which fixes the issue, whereas rafThrottle does not.
@StevenDufresne
Copy link
Contributor

In regards to the footer overlap, since the TOC shows up at the right level relative to the document, could we utilize position: sticky as opposed to position: fixed? That would remove the need for js.

Example:
https://d.pr/i/HZveB8

In the example, I just changed to sticky positioning and relied on a negative margin-right to pull it right.

@adamwoodnz
Copy link
Contributor Author

In regards to the footer overlap, since the TOC shows up at the right level relative to the document, could we utilize position: sticky as opposed to position: fixed? That would remove the need for js.

Example: https://d.pr/i/HZveB8

In the example, I just changed to sticky positioning and relied on a negative margin-right to pull it right.

Hmm nice idea, I'll try it out thanks

@adamwoodnz adamwoodnz force-pushed the fix/6475-toc-overflow branch 2 times, most recently from a28e6ee to 08a810d Compare October 10, 2022 03:11
@@ -2243,7 +2243,6 @@ div[class*="-table-of-contents-container"] {
background: linear-gradient(to right, #fafafa 35%, #fff 35%);
max-width: 100%;
padding: 0;
overflow: auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A parent with this property stops position: sticky working on children. I'm unsure why it's being set and haven't observed any changes as a result of removing it.

@adamwoodnz
Copy link
Contributor Author

👍 Seems to work well @StevenDufresne, please give it a try

@StevenDufresne
Copy link
Contributor

Yep, this is working as expected. Good job!

@adamwoodnz adamwoodnz merged commit 34241c1 into trunk Oct 11, 2022
@adamwoodnz adamwoodnz deleted the fix/6475-toc-overflow branch October 11, 2022 20:16
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.

None yet

2 participants