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

Resolve scrollbar visibility on navbar on long content #215

Merged
merged 1 commit into from
Sep 9, 2015

Conversation

agjohnson
Copy link
Collaborator

This fixes #200, where a scrollbar was sometimes visible on the navbar. This
unfortunately wasn't addressable with just CSS, as outlined in #206. Because we
need the element to be scrollable, we can't set overflow: hidden on the nav
element.

This fixes this issue by:

  • Adding a wy-side-scroll element over the fixed position nav element and
    under the menu item elements
  • wy-side-scroll is set to 320px width, while the fixed position nav elements
    and menu item elements are 300px, clipping the scrollbar with overflow-x: hidden on the fixed position element
  • Javascript is set to scroll the new scroll element instead of the parent
    fixed position element
  • For backwards compatibility on already generated HTML, the new scroll element
    is added dynamically if it is missing, and children of the fixed position
    element are moved there.

This was tested to be working in both cases on a variety of platforms: Linux FF,
Chrome, Windows IE, OSX Chrome and Safari, iPhone 5.1, and Android 4.2.

@agjohnson agjohnson added this to the v0.1.9 milestone Jul 17, 2015
@ericholscher
Copy link
Member

I'm not really qualified to review this, but would love if someone else has thoughts. Changing the HTML format of all of our docs is a relatively large change, but if it's required for this functionality to work, so be it.

@takluyver
Copy link

The JS to dynamically modify the HTML if it's out of sync with the new CSS seems like a pretty... exciting approach. ;-)

Presumably this is only an issue on RTD, right? For people building with the theme locally, the HTML and the CSS should always be from the same version of the theme, so tricks like this shouldn't be necessary. If that is the case, it seems like the JS to dynamically tweak the HTML should be in RTD itself, not in the theme code.

RTD could also do a couple of things to reduce the need for things like this:

  • Versioned URLs for the CSS, so that docs always use the CSS matching the theme version they were built with.
  • Trigger rebuilds of docs to move them to the new theme. I guess doing this for all projects would take quite a while, but maybe it's possible to use some intelligence to prioritise docs that are viewed frequently and/or less likely to have a rebuild triggered soon anyway.

@agjohnson
Copy link
Collaborator Author

This would be an issue for RTD, but also possibly for forks of this theme that aren't pinned to 0.1.7.

I lean towards keeping it here, as RTD is the main use case for this theme. It will definitely have to live on RTD either way however. Our opinion is we shouldn't force a rebuild on documentation for this. Versioned assets are on our roadmap, but would require some significant changes to be able to handle all the cases that we need to handle on RTD.

If we change the HTML and don't provide graceful degradation here, this should be a backwards incompatible version.

@agjohnson agjohnson mentioned this pull request Aug 26, 2015
This resolves readthedocs#200, where a scrollbar was sometimes visible on the navbar. This
unfortunately wasn't addressable with just CSS, as outlined in readthedocs#206. Because we
need the element to be scrollable, we can't set `overflow: hidden` on the nav
element.

This fixes this issue by:

 * Adding a `wy-side-scroll` element over the fixed position nav element and
   under the menu item elements
 * `wy-side-scroll` is set to 320px width, while the fixed position nav elements
   and menu item elements are 300px, clipping the scrollbar with `overflow-x:
   hidden` on the fixed position element
 * Javascript is set to scroll the new scroll element instead of the parent
   fixed position element

This was tested to be working in both cases on a variety of platforms: Linux FF,
Chrome, Windows IE, OSX Chrome and Safari, iPhone 5.1, and Android 4.2.
@agjohnson
Copy link
Collaborator Author

Okay, i've just moved the javascript into RTD's extension of this javascript as a module in a separate branch. I'm going to target this for a backwards incompatible release, which will be v1.0 based on semvar.

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.

Navbar scrollbar intermittently shows
3 participants