search

PreTeXtBook / pretext

PreTeXt: an authoring and publishing system for scholarly documents
https://pretextbook.org
Other
268 stars 210 forks source link

Javascript: improve TOC scroll behavior #2282

Closed oscarlevin closed 3 weeks ago

oscarlevin commented 4 weeks ago

Currently, if you have a long table of contents and you click on the third entry, the javascript scrolls that to the very top of the page, hiding the first two entries. I propose that it would be better to only scroll if the item clicked on is near the bottom (in this case, below the bottom third of the page). This has the advantage of always showing elements above and below the highlighted division, which the reader would likely want to click on.

Pinging @davidfarmer just to make sure he is okay with this.

rbeezer commented 4 weeks ago

Not too much for me to review, but this could go in with a (careful) look from @davidfarmer.

I'm not sure I see this behaving as advertised, on Firefox. But I might not understand right.

While experimenting - before and after this PR, locally and at website - ToC on sample article has a few problems, I think. For example, selecting Section 19 scrolls ToC to very top, and highlighted Section 19 is not in view. Maybe misbehavior begins around Section 17 and 18. Maybe.

davidfarmer commented 4 weeks ago

In principle this seems good, and the code change looks okay, but I have not tested it.

Simple enough to revert in the unlikely case there is an undesirable side effect.

oscarlevin commented 4 weeks ago

I just tested with firefox and it seems to be working, although perhaps it could scroll a little higher there. I don't know why it isn't scrolling on chapter 19 though. The toc doesn't scroll without the change either for that chapter.

ascholerChemeketa commented 4 weeks ago

It looks like the window.addEventListener("load" calls on lines 56 & 61 could be document.addEventListener("DOMContentLoaded". That would speed up the triggering of the code and it looks like fix the Firefox issue. (See https://stackoverflow.com/questions/69503436/why-load-event-comes-before-domcontentloaded for info on race condition that is probably causing the issue.)

The context around the item is a big improvement. But I think the "target" should end up closer to the top than the bottom to make it more prominent. Maybe 25% or 33% down instead of 66%?

oscarlevin commented 4 weeks ago

I've implemented @ascholerChemeketa's suggestions. Assuming that the two commits will be squashed into one.

rbeezer commented 3 weeks ago

Much, much better. Thanks for the collaboration on this one.