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

Fix always showing dead scrollbar in flyout #835

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

agjohnson
Copy link
Collaborator

@agjohnson agjohnson commented Oct 18, 2019

Stop always showing a scrollbar on the flyout menu, though preserve flyout menu scrolling if the content doesn't fit in the viewport.

This one has bothered me for a while. At some point we introduced an always-on scroll bar:
image

For the vast majority of cases, the scroll bar isn't needed -- this content would only be scrollable if you had a large number of versions or your viewport was very short.

This makes only the flyout menu content scrollable and it makes the scrollbar only show when needed. It gets rid of the scrollbar over the .rst-current-version header element -- this element shouldn't be part of the scrollable content.

image

In order to do this, we control the version menu height from the child .rst-other-versions instead of from the parent .rst-versions element. We can't force the height on the parent .rst-versions like we were, because that makes it much harder to also control the height (and set the content to scrollable) on the child .rst-other-versions at the same time. Controlling the spacing and positioning of the scrollable content is very hard with the part div height set.

So instead, we set the height at the child .rst-other-versions and let that dictate the height of the .rst-versions div. Because this div doesn't include the height that the .rst-current-version header element takes up, we can't set height: 100%, or the .rst-current-versions element would not be visible on flyout menus that exceed the viewport height.

Instead, 80vh or 90vh is used here for a max-height, based on a media query. This is an approximation to include the .rst-current-version header element in our height calculation -- at viewport breakpoint height of 410px, 90vh = 369px, leaving 41px (the height of the .rst-current-version element).

Ideally, we could restructure the html to allow .rst-current-version to float in a way that allowed the child .rst-other-versions to occupy the full height of the .rst-versions parent element. We can sett max-height: 100vh or perhaps max-height: 100% in this case.

scroll-bar-fix

Fixes #791

Stop always showing a scrollbar on the flyout menu, though preserve
flyout menu scrolling if the content doesn't fit in the viewport.
@agjohnson agjohnson requested a review from a team October 18, 2019 06:45
@agjohnson
Copy link
Collaborator Author

I still need to test badge only on this. I'll set WIP until i do.

@agjohnson agjohnson added Bug A bug Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review labels Oct 18, 2019
Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

This appears to mess up the padding on the badge only, the "Read the docs" text and the version on the right do not line up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flyout menu has a scrollbar
3 participants