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

Projects
None yet
3 participants
@agjohnson
Collaborator

agjohnson commented Jul 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Jul 20, 2015

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.

Member

ericholscher commented Jul 20, 2015

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jul 21, 2015

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.

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

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Jul 22, 2015

Collaborator

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.

Collaborator

agjohnson commented Jul 22, 2015

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 referenced this pull request Aug 26, 2015

Merged

Fix modernizr url #195

Resolve scrollbar visibility on navbar on long content
This resolves #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

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

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 9, 2015

Collaborator

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.

Collaborator

agjohnson commented Sep 9, 2015

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.

@agjohnson agjohnson modified the milestones: v1.0, v0.1.9 Sep 9, 2015

agjohnson added a commit that referenced this pull request Sep 9, 2015

Merge pull request #215 from agjohnson/fix-nav-scrollbar
Resolve scrollbar visibility on navbar on long content

@agjohnson agjohnson merged commit 2992ca2 into rtfd:master Sep 9, 2015

@agjohnson agjohnson modified the milestones: v1.0, v0.1.9 Sep 9, 2015

@pyup-bot pyup-bot referenced this pull request Jul 4, 2017

Open

Initial Update #1

@pyup-bot pyup-bot referenced this pull request Sep 29, 2017

Closed

Initial Update #1

@pyup-bot pyup-bot referenced this pull request Feb 7, 2018

Merged

Initial Update #4

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