Fix: Scroll sidebar to current active section (#1067) #1068
Conversation
|
@morphologue 👍 👍 |
|
I like the idea in theory, but Is there some other way to accomplish it without |
Yes, I love Firefox. |
There was a problem hiding this comment.
Good idea to improve the UI, @ngolin 👍 By way of contrast, my change (#1052) was just directed at fixing the existing code. There was already code there to scroll to the top, but it had been broken by a change to the HTML structure.
| if (activeSection) { | ||
| sidebarScrollBox.scrollTop = activeSection.offsetTop; | ||
| } | ||
| window.addEventListener('load', function() { |
There was a problem hiding this comment.
Is it necessary to add an event listener here? This script is loaded at the end of index.hbs after all the HTML for the sidebar, so that's why the existing code was able to directly query for the .active element.
There was a problem hiding this comment.
I'm just too careful. Just querying the .active element is not enough, maybe at the end there are some CSS or JS change the width or height of elements dramatically, which would break the scrolling. So I wait until the page totally rendered, that is window loaded.
There was a problem hiding this comment.
It's a good instinct to be careful, however the load event doesn't guarantee that JavaScript has finished manipulating the DOM. It just means that HTML and dependent resources (such as images and stylesheets) have been loaded: https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event. Given that there is other code in this file which assumes that the DOM has stabilised, I think this event listener is a bit unnecessary. Of course it's not harmful though either (unless some images take a long time to load).
There was a problem hiding this comment.
The addition of this event listener causes significant jumping and flickering on chrome and safari browsers. Would it be possible to remove it?
|
I'm going through the rust book at the moment and this is driving me nuts! Glad to see a PR has already been submitted and reviewed. Any idea when it'll get merged? |
Hi @trstruth, my PR #1052 was merged on the 6th of October and included in release v0.3.2 The problem is that the public Rust book seems to have been built using an older version. I am temporarily hosting a version of the Rust book with my fix which you might find more convenient for now. In case you're wondering about the difference between this PR and mine, mine just fixed the existing code (which was trying to make the active selection show at the top of the screen), whereas this PR makes more substantial changes, and the active selection would show in the middle of the screen. |
|
@morphologue thanks for the info, I've opened an issue on the rust book repo describing the situation. Will use your temporary host in the meantime :) |
I also host all the books here, all compiled from the master. |
|
any update on this PR, when this will be merged? |
|
@avinassh I'm waiting for the fixes for the addition of the event listener (#1068 (comment)). I've been intending to make the changes myself when I have the time, but I haven't been able to, yet. If someone wants to resubmit the PR with the necessary fixes, that would be great. It would also be useful to fully test this on a wide array of browsers and platforms, and report how well it works. |
Sidebar on the left can't auto scroll to the current active section. This is something to do with the
book.js.Edit: I just aware that there already has been a PR #1052 fixing this problem. But I think my PR is a bit cleaner and using the nice Web API. My solution places the active item in the middle of the sidebar whenever possible, which I think it's a bit nicer. 😉