-
Notifications
You must be signed in to change notification settings - Fork 482
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
Fix scroll and put nav visually "in front" for narrow/mobile screens #863
Conversation
assets/html/documenter.css
Outdated
} | ||
|
||
nav.toc > ul { | ||
overflow-y: scroll; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, I think we could just use overflow-y: auto
, so the scroll bar only shows up when needed. But I don't know if there was a rationale behind using scroll
on nav.toc
before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think auto
looks better actually. The scrollbar, at least with the -webkit-*
styling I applied, creates this visible bar next to the currently selected item part, which nicely goes away if there is no need for a scrollbar.
assets/html/documenter.css
Outdated
@@ -560,7 +564,7 @@ article section.docstring a.source-link { | |||
} | |||
|
|||
article > header div#topbar span { | |||
position: fixed; | |||
position: relative; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The position: fixed
has always seemed like a bug to me on mobile. I think this is the correct fix to make it properly hide along with the menu when scrolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to work properly like this (on Chrome), but I agree that it doesn't make much sense logically. As we're not setting any position values, couldn't we get rid of position
altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! 👍 for the shadow! Let me know when it's good to go.
assets/html/documenter.css
Outdated
@@ -560,7 +564,7 @@ article section.docstring a.source-link { | |||
} | |||
|
|||
article > header div#topbar span { | |||
position: fixed; | |||
position: relative; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to work properly like this (on Chrome), but I agree that it doesn't make much sense logically. As we're not setting any position values, couldn't we get rid of position
altogether?
assets/html/documenter.css
Outdated
} | ||
|
||
nav.toc > ul { | ||
overflow-y: scroll; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think auto
looks better actually. The scrollbar, at least with the -webkit-*
styling I applied, creates this visible bar next to the currently selected item part, which nicely goes away if there is no need for a scrollbar.
@mortenpi Checks are passing, looks like it's good to go! 💜 |
Sorry, I should've caught this earlier, but after #792 on desktop, the
overflow: scroll
adds an additional visible, but unusable, scroll bar for narrow screens! 😄 I took the opportunity to also "lift up" the navigation menu, by removing the inset from the shadow in this case, as I think there is something visually unappealing about a sliding menu, that looks sort of like it folds the page backwards on itself.