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

Updates to mkdocs ReadTheDocs theme #1156

Closed
wants to merge 6 commits into from
Closed

Conversation

@ghost
Copy link

@ghost ghost commented Mar 10, 2017

This Pull Request addresses three issues: 1) adjusts TOC scrollbar so that the bottom is not covered by the 'Next' navigation button; 2) fixes the 'GitHub' class name in the version.html file; and 3) incorporates @seanmadsen code posted on Jan 10 issue #803 (if accepted and merged can be marked as closed) which may address some concerns with issue #588. Still needing someone to address the collapsible TOC headings discussed in issue #588.

mkdocs github update scrollbar

SquareWaffles added 4 commits Mar 10, 2017
GitHub link was not showing.  Found out that the class name should be "fa fa-github".
This built upon previous code so that when the scrollbar is viable the bottom arrow is not covered by the "Next" navigation button.  This is actually an improvement over ReadTheDocs as the same overlapping behavior happens there.
SquareWaffles
The code is a copy of code posed on Jan 10, 2017 on issue 803.  This just incorporates the auto scroll into the theme itself without the need for additional custom.js file.
SquareWaffles
The theme_extra.css file failed Travis CI Build check because of a '0%' on 'min-height'. Changed to '0'.
@waylan
Copy link
Member

@waylan waylan commented Mar 10, 2017

At a quick glance this looks good. Will take a closer look when I have some time.

However, while working on #1155 earlier today I noticed that the upstream (Sphinx) theme does not even have a visible scrollbar for the side nav. See this page for example. Perhaps the correct thing here is to do the same rather than the scrollbar adjustment here.

@ghost
Copy link
Author

@ghost ghost commented Mar 11, 2017

Ah, I did not even notice that Sphinx does not have a scrollbar on the toc. Further investigation into when the scrollbar appeared in the mkDocs RTD theme references Pull Request #202. There wasn't any conversation as to if the scroll should be visible. I agree with your other posts that the mkDocs RTD theme should resemble Sphinx as much as possible. But would it be advantageous to implement minor improvements where we can? Yes, I know that then rabbit holes into defining "minor improvements".

The issue with the absence of the scroll is that depending on your window size you may not know that there are more headings in the overflow. For instance, when I opened the link you referenced, my default window size makes it appear that the toc ends after "Ethical Advertising". Furthermore, some machines (mine included) do not have scrolling enabled, therefor we use the scrollbar and the up/down keys. The auto focus is on the main content as it should be so I have no idea that there is an overflow in the toc when using the keys. Only way I can scroll in the toc is to click somewhere that is not a link and then use they up/down keys. If the scrollbar is visible (only when content exceeds the view), it then serves two purposes: 1) alters the reader that there is more content and 2) allows easy access to the content.

So, this all gets back to, how far should we deviate from Sphinx? If you want it as close to possible, then this pull request should be rejected as Sphinx does not scroll in the toc and does not auto focus the toc on load. I can see the advantages of keeping this theme as close to Sphinx as possible and I can see the advantages of implementing minor improvements. The decision is ultimately up to you and the direction you want to take.

@waylan
Copy link
Member

@waylan waylan commented Mar 11, 2017

@SquareWaffles You make good points. I think for now we will leave things as-is, which means accepting these improvements.

Now regarding these changes, I took a closer look and I found one problem. When in "mobile mode" (narrow browser window), there is a blank space at the bottom of the nav bar:

screenshot composite

Note that to see this, you need to narrow the browser window until the nav bar disappears, then click the hamburger button in the top left corner to show the nav bar. In that case, the nav bar should go all the way to the bottom of the window as the next/previous bar is at the bottom of the main body rather than the bottom of the nav bar. Of course, this makes sense for someone browsing on their phone. Requiring them to use the hamburger button to get to next/previous would be ridiculous.

SquareWaffles added 2 commits Mar 11, 2017
SquareWaffles
Added override for the mobile TOC to be "height:100%" so it does not account for the previous/next navigation buttons that are visible on the desktop view.
SquareWaffles
The update failed Travis CI Build check because of a forgotten space between ".wy-nav-side" and ".shift{".  Space is now added.
@ghost
Copy link
Author

@ghost ghost commented Mar 11, 2017

Thanks for catching that @waylan. The css has been updated and ready for re-testing.

@waylan
Copy link
Member

@waylan waylan commented Mar 11, 2017

That doesn't seem to solve the problem for me (checked both firefox and chrome). Besides, the shift class is added to the nav div when activating the hamburger button, but it doesn't go away when the window resizes. So, for example, rotating a tablet from portrait to landscape would retain the same 100% height, but mobile mode would be deactivated --that is, if it worked at all, which it doesn't for me for some reason I can't quit make out. The browser doesn't seem to even acknowledge that the rule was defined, let alone have it override the previous rule.

In any event, I think this makes more sense:

@media screen and (max-width: 768px) {
    .wy-nav-side {
        height: 100%;
    }
}
@waylan
Copy link
Member

@waylan waylan commented Mar 11, 2017

I just cleaned up the commit history (swashed the fixes for Travis failures) and incorporated my fix above. @SquareWaffles thanks for your help on this. I'm closing this in favor of #1157 (don't worry, you are still listed as author in the individual commits).

@waylan waylan closed this Mar 11, 2017
kylegregory added a commit to kylegregory/mkdocs that referenced this pull request Mar 23, 2017
Fixes a JavaScript error when looking for a current page in the TOC
when there is not a current page selected, in the case of Search, 404
or any page that isn’t listed in the TOC. This checks if a TOC item
with the class of ‘current’ is in the document, and if not, does not
call the viewport check or scroll the TOC area.

Addresses mkdocs#1177, introduced in mkdocs#1156 and referenced in mkdocs#803.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.