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

IntersectionObserver at 1/3 screen #567

Merged
merged 3 commits into from
Jul 19, 2022

Conversation

choldgraf
Copy link
Member

@choldgraf choldgraf commented Jun 4, 2022

This adjusts our IntersectionObserver so that the Table of Contents section hides itself when margin content is 33% up from the bottom of the screen, rather than as soon as it shows up on the screen at all.

This should hopefully be a nice balance between "it doesn't hide soon enough and so blocks content" and "it hides too aggressively / quickly" as reported in:

I chose 33% as it seemed to be a reasonable point at which a reader might actually want to look at some margin content.

cc @spring-haru - think this is a nice compromise?

closes jupyter-book/jupyter-book#1729

@pradyunsg
Copy link
Member

Is it not possible to base it on the height of the table of contents div?

@pradyunsg
Copy link
Member

pradyunsg commented Jun 4, 2022

It should be possible -- adding elements to onScreenElements only if entry.target.getBoundingClientRect().top >= (<the Element object for .bd_toc from a document method>).getBoundingClientRect().bottom.

@choldgraf
Copy link
Member Author

But I think that you'd then need to trigger the callback on a scroll event rather than using intersectionobserver right?

@pradyunsg
Copy link
Member

pradyunsg commented Jun 4, 2022

IIUC, the callback is triggered as you scroll -- https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API#result?

@pradyunsg
Copy link
Member

Oh, and we also wanna be checking whether the event.target's bottom is higher than the bd-toc's top?

@choldgraf
Copy link
Member Author

Hmmm I should read up on that more...i had thought it only triggers the callback when the target element intersects with the root for the first time, but maybe i am mis remembering! That would indeed be a nicer fix here

@pradyunsg
Copy link
Member

I could very well be wrong -- I've only skimmed the page.

If it only triggers on the edges, we can still probably handle this by keeping a list of "things on screen" using IntersectionObserver and checking only those margin items to see if they would collide with bd-toc (w/ a scroll handler that has a debounce + once-per-n-milliseconds throttle).

@choldgraf
Copy link
Member Author

I spent a little while looking at this, and I do think what @pradyunsg suggests is possible, though I am not sure it is worth the penalty we'd pay in complexity.

There are two ways that we could ensure that we only hide the TOC when it intersects with margin content:

  1. Do a rate-limited scroll event
  2. Set up a grid of IntersectionObservers with different margins (e.g., 0, -20%, -40%, -60%, etc)

In both cases we'd use the old-fashioned way of "getClientBoundingRect()" and do a manual check for their intersection.

However, I'm still inclined to just go with the "1/3 up the page" approach here because it would be much simpler, and I don't know that it would be that big of a deal if the toc didn't quite hide early enough.

I guess the things we're trying to avoid here are:

  1. Margin content is hidden by the TOC in a moment where the person might want to read it
  2. The TOC is hidden too aggressively so is annoyingly never present even when it seems like it isn't useful

I think that by making the "hiding" action only trigger 1/3 of the way up the page:

  1. We avoid 1 as long as:
    • The TOC wasn't > 2/3 of the page in height
    • If it was, the reader wouldn't need to read the margin content if it was < 1/3 up the page
  2. We avoid 2 by letting margin content exist in the bottom third of the page, so the TOC only hides once it seems like it is getting closer

Do others agree w/ that tradeoff of complexity and UX? 🤔

@choldgraf
Copy link
Member Author

I'm going to merge this one in as an iterative improvement - we can track making an update to do a more proper "intersection check" in another issue to figure out the costs / benefits.

@choldgraf choldgraf merged commit 9615c95 into executablebooks:master Jul 19, 2022
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.

Margin content hides the table of contents even if they don't directly overlap
2 participants