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

Make the navigation bar automatically scroll to the selected item #792

Merged
merged 6 commits into from
Oct 10, 2018

Conversation

dalum
Copy link
Contributor

@dalum dalum commented Aug 13, 2018

I am not sure if I made the change in the right place, but it works locally. If the navigation bar is really long (as in base), I find it slightly inconvenient that the navigation view jumps to the top when clicking a menu item. This PR shifts the navigation view to the current item at the end of the document.ready event call.

@mortenpi mortenpi added this to the 0.20.0 milestone Aug 13, 2018
@mortenpi
Copy link
Member

Thanks! This would be great to have!

My only concern is that it always hides the logo and the search box (you can scroll up, but you need to know that there's something there in the first place). Perhaps only the actual menu should scroll? A more involved version could then hide the the logo (but perhaps not the search) when the user starts scrolling down in the menu (like we do on mobiles with headroom).

@dalum
Copy link
Contributor Author

dalum commented Aug 14, 2018

It took a bit of CSS fiddling to get it to work, but now it always shows the logo and search. I could try to make scrolling the menu hide them, but I feel that it's more of a gimmick than a feature, and might be annoying to some.

@mortenpi
Copy link
Member

What do you think about subtracting 1.5*$(".current > a").outerHeight() from the scrollTop, in order to have a bit of the menu above the current item visible, giving the user another visual cue that there is something up there?

Actually, alternatively, we wouldn't have to keep the logo and stuff fixed at all if we'd try to make sure that the gray box is symmetrically in the middle. "Home" is unlikely to be very tall, so it would keep the logo visible. Any thoughts?

@dalum
Copy link
Contributor Author

dalum commented Oct 3, 2018

@mortenpi Sorry, I haven't looked at this in a while, but I tried subtracting the 1.5*... offset, and I have to admit I don't like it that much. Same with the symmetrical placement. To me it looks like the offset computation errored somehow. 😏 I was also just reading the Rust book (https://doc.rust-lang.org/stable/book/2018-edition/) and noticed that their table of contents scrolls so the currently selected item is at the top, and in my opinion, it looks better and gives a feeling of "momentum" or progress, if you are reading the manual linearly. Which of course isn't always the best way to read a manual, so that is worth considering. To solve the search bar issue, the Rust book has it nicely tucked away as a drop–down in the main document.

I think the least invasive compromise is probably to make just the search bar hover when scrolling down.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Thanks! I took another look at this now and I think it looks great as is. The scrollbar on the right is actually more than enough to indicate that that there are menu items above.

Keeping the logo and search fixed is a good compromise I think. We can always improve on that in the future.

If it's good to go on your part I'll merge this soon.

@dalum
Copy link
Contributor Author

dalum commented Oct 9, 2018

Good to go! 😊

@mortenpi mortenpi merged commit 65fb22f into JuliaDocs:master Oct 10, 2018
mortenpi added a commit that referenced this pull request Oct 10, 2018
@mortenpi
Copy link
Member

It seems that this broke the version selector (ref #855), which I missed since the local build didn't have a selector. Somehow the <select> gets resized, even though it has an explicit height.

@dalum
Copy link
Contributor Author

dalum commented Oct 12, 2018

Oh, my local build also didn't have the selector. I can try and look into a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants