Skip to content

feat(theme/stylus/menu): Make sticky #551

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

Merged
merged 7 commits into from
Jan 19, 2018
Merged

feat(theme/stylus/menu): Make sticky #551

merged 7 commits into from
Jan 19, 2018

Conversation

sorin-davidoi
Copy link
Contributor

Questions:

  • should we add a faint border at the bottom of the menu (to separate it from the content)?
  • should we implement auto-hiding when scrolling down?

Closes #545.

@Michael-F-Bryan
Copy link
Contributor

should we add a faint border at the bottom of the menu (to separate it from the content)?

I dunno, I kinda like the ghostly effect where the menu buttons just float on top of the page. What about trying it then taking a screenshot so we can see how things look with and without the border?

should we implement auto-hiding when scrolling down?

Hmm... Wouldn't that kinda defeat the purpose of having the menu sticky?

@sorin-davidoi
Copy link
Contributor Author

Wouldn't that kinda defeat the purpose of having the menu sticky?

Don't think so. It would save screen space when reading (scrolling down). As soon as you want to reach for the header actions and start scrolling up (let's say more than 10px), the header would appear, saving you from the trouble of swiping multiple times to reach it.

@Michael-F-Bryan
Copy link
Contributor

as soon as you want to reach for the header actions and start scrolling up (let's say more than 10px), the header would appear

Ah okay, I understand what you're talking about. For non-mobile users, can we also add a way that it'll show itself if you move your mouse up to the top of the page as well? Similar to what we currently do with fading in/out the title. It'd be annoying to lose your header on a laptop and then have to scroll up to show it again, whereas that's a pretty natural operation on mobile.

@sorin-davidoi
Copy link
Contributor Author

For non-mobile users, can we also add a way that it'll show itself if you move your mouse up to the top of the page as well?

That might prove to be annoying - if I want to click on a link on the top of the page just to find that the menu gets in the way.

I've added the bottom border and the autohide functionality when scrolling up, let me know what you think.

image

@sorin-davidoi
Copy link
Contributor Author

Similar to what we currently do with fading in/out the title.

Why exactly do we do that? I find it kind of strange, there is this big gap up there.

@Michael-F-Bryan
Copy link
Contributor

I just checked this PR out onto my laptop and I think the overall idea is pretty good.

I'm not so certain about adding a bottom border to the menu bar though, it kinda breaks up the page and puts the content in a box, making it feel less "fluid"... Does that make sense?

Why exactly do we do that? I find it kind of strange, there is this big gap up there.

I think it's for similar reasons to why we're hiding the menu when you scroll down. To keep the UI as free of clutter as possible. If we're autohiding the menu I think we should remove the fade in/out effect, seeing as it's now redundant.

That might prove to be annoying - if I want to click on a link on the top of the page just to find that the menu gets in the way.

After experimenting a bit, I think this works alright. Once you're reading, you don't usually want to touch anything on the menu anyway.

@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jan 17, 2018

I'm not so certain about adding a bottom border to the menu bar though, it kinda breaks up the page and puts the content in a box, making it feel less "fluid"... Does that make sense?

It does. One thing we could try is show it only when the menu is sticky (scrollTop > 0).

I think we should remove the fade in/out effect, seeing as it's now redundant.

👍

After experimenting a bit, I think this works alright. Once you're reading, you don't usually want to touch anything on the menu anyway.

This is a little tricky to implement. Since the menu will be off-screen, we can't just listen for the hover event on it. I think we would need to listen for all mouse events and check if the y coordinate < 50px. It is possible to do without JavaScript 🎉 .

@sorin-davidoi
Copy link
Contributor Author

Alright, implemented show-on-hover. It leads to some more complicated selectors though:

html:not(.sidebar-visible) #menu-bar:not(:hover).folded > #menu-bar-sticky-container

My only concern is how will this feel on mobile devices, where :hover styles are only applied on-tap.

@Michael-F-Bryan
Copy link
Contributor

I just tried this out on my laptop and I think you've found a happy middle ground 👍

The header will remain shown when you've got the sidebar open, but then when it's closed you go into an "uncluttered reading mode" where everything tries to get out of your way and just show you the chapter content. As you said might happen, it can sometimes be annoying trying to click a link which is within the top 50px or so. Depending on how often people encounter that it may turn into a papercut.

I also tried it out on my mobile and found hiding the top bar when you scroll down quite intuitive. It matches the behaviour of lots of other mobile sites.

I'm pretty happy with this PR. Is there anything else you'd like to do, or is it ready to merge?

@sorin-davidoi
Copy link
Contributor Author

No, I would say this is all for now.

@Michael-F-Bryan Michael-F-Bryan merged commit 80f4267 into rust-lang:master Jan 19, 2018
@sorin-davidoi sorin-davidoi mentioned this pull request Jan 19, 2018
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
* feat(theme/stylus/menu): Make sticky

* feat(theme/stylus/menu): Faint bottom border

* feat(theme/book): Auto hide menu when scrolling down

* feat(theme/stylus/menu): Don't hide title

* feat(theme/stylus/menu): Only show bottom border when sticky

* fix(theme/stylus/menu): Don't hide when sidebar is expanded

* feat(theme/book): Show menu bar on hover
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.

2 participants