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 cut-off navigation in the ReadTheDocs theme #2297

Merged
merged 7 commits into from
Apr 9, 2021

Conversation

nathanlesage
Copy link
Contributor

Hey, first of all thank you for this package, which is really, really helpful for easily writing docs. I just stumbled upon issue #2012 this morning, and decided to fix that.

This PR addresses this by adding miniscule changes to the base CSS of the ReadTheDocs theme.

Cause of the issue

This issue is caused by the navigation container .wy-nav-side having a padding-bottom of 2em, while the container .rst-current-version simply uses a relative font-size of 90% in addition to a padding of 12px above and below. These two need not result in the same sizes. In fact, the .rst-current-version container will always be a little bit higher than the 2em padding on the .wy-nav-side container.

Fix

I fixed this by assigning the .rst-current-version container a fixed height of 40px and applying that height as a padding-bottom to the .wy-nav-side container. This way, the navigation container will scroll exactly up to that point, so the last nav-item will exactly end where the .rst-current-version container will start.

Additionally, in order to retain the items's vertical alignment and the overall appearance, I switched the .rst-current-version container to a flexbox layout, accounting both for possibly missing repo_name configuration and for being on the first or last page. This additionally involved "cleaning up" the inline-styles that were applied to the anchor and span elements within that container, so I modified versions.html as well.

Tests

I have tested the effects of my amendments by applying the items in DevTools using a running MkDocs instance. (In fact, I have fixed the issue by applying the styles, and then copied these over to the theme_extra.css.)

Documentation

I have added extensive documentation to what my changes do following the style of previous comments in the theme_extra.css. I did not add documentation to versions.html, but referenced the changes made there in the documentation within the extra CSS file.

Caveats

I should note that I somehow did not manage to get MkDocs to apply those changes during testing. I have forked, downloaded, and installed my fork of mkdocs using a virtual environment and using the pip install --editable .-command, which did in fact put MkDocs into my path as it should've been. However, the changes were somehow not propagated to a served instance on my computer.

I personally credit that to my lacking knowledge of Python's package environment, but hope that you can confirm that these changes work with the correct setup. If not, I would be more than happy to help getting these to work with your help. Please excuse my lack of knowledge with Python in this regard.


Small note: I just figured we could move the color-inline styles from the versions.html into the theme_extra.css as well. I didn't add this as well to keep the changes minor, but this would make sense. One could simply add this to the last statement in the theme_extra.css, below flex: 1;.

@waylan
Copy link
Member

waylan commented Feb 8, 2021

Thanks for your work on this.

Note that this theme is a clone of the Sphinx theme of the same name. For the most part, we simply copy the CSS and JavaScript as-is and then fashion our templates to output (mostly) the same HTML as the upstream theme. The only additional CSS we include in theme-extra.css is generally to make up for deficiencies where we are not able to match the upstream HTML (for example, Markdown tables differ from Rst tables in subtle ways).

So thanks for locating your changes in the correct location. However...

If this bug does not exist upstream, then my assumption is that our HTML is missing something (maybe a css class on an element or a wrapping element). Also, the fact that the HTML includes inline styles makes me wonder if that is because the upstream theme does as well. To be clear, I haven't checked on either in this case but it does give me pause and should probably be investigated before moving forward with this fix.

It is also possible that this bug did exist upstream but has since been fixed, in which case updating to a more recent version of upstream may be the best way to resolve the issue. Again, that should probably be investigated. That said, if this is the case, it may be quicker for us to apply your fix temporarily with a note in the comments to remove the fix when we update to a more recent version of upstream.

Finally, I am concerned about your inability to see your changes reflected in a served instance. Installing with --editable in a virtualenv is the correct way to accomplish that. Is it possible that the rules were overridden by the upstream rules due to having a lower specificity? If so, they should show up as disabled in your browser's inspect tool. Have you checked that? Or are they not showing up at all?

@nathanlesage
Copy link
Contributor Author

Thank you for the extensive reply.

If this bug does not exist upstream, then my assumption is that our HTML is missing something (maybe a css class on an element or a wrapping element)

I just checked, and in fact, the Sphinx demo instance has an additional specification that states a padding-bottom of even 3em:

image

However, I don't think that this would solve the issue. Personally, I think that having fixed sizes that match each other are better (because this way, the last navigation item perfectly aligns to the current-version container).

Furthermore, MkDocs does not make use of the versions container in a way that the Sphinx theme does. There, you can actually expand the container to display further versions to switch between those:

image

So I think it's fine to stick with these "monkey patches," because they work in every MkDocs configuration, as no other divs are in any way appended to that container. Yes, these might break in the future, but that breakage would be detected while testing the new upstream CSS rules, and fixing those is not that difficult, fortunately.

That said, if this is the case, it may be quicker for us to apply your fix temporarily with a note in the comments to remove the fix when we update to a more recent version of upstream.

I agree. I have documented what I changed in every inch, so removing the changes is fairly straightforward.

Finally, I am concerned about your inability to see your changes reflected in a served instance. Installing with --editable in a virtualenv is the correct way to accomplish that.

So, I'm using conda locally because I use Python almost exclusively for statistics and machine learning in my PhD. I created a new environment using conda, and this means that my PATH pointed towards the environment pip.

I did not have any MkDocs installed (the local fork was the only one) and the mkdocs command correctly points to a directory in said environment. This means I don't really know where this came from, because the theme_extra.css was the original file in every respect, my changes were simply not there. (I did double-check the files, of course!) My changes definitely work in the way I implemented these, but that was my reason for indicating that I may lack some knowledge with regard to Python. I simply have given up understanding the different ways of handling Python :/

@waylan
Copy link
Member

waylan commented Feb 8, 2021

MkDocs does not make use of the versions container in a way that the Sphinx theme does.

Ah, right. That is the difference which causes the bug. You're right, in this situation, patching the CSS rules is the way to go.

I'm using conda locally

Okay, that explains the issue. Conda does some weird stuff that I don't understand. We have occasionally received reports, but never with enough information to narrow down to something actionable. We would need something with a Conda environment to work on the issue. Until then, I don't recommend development on MkDocs in a Conda environment.

In any event, when I get some time, I'll pull your changes locally and confirm they work as advertised. So long as they do, I'll merge this. In the meantime, a note needs to be added to the release notes.

@nathanlesage
Copy link
Contributor Author

Okay, that explains the issue. Conda does some weird stuff that I don't understand.

Feel you. I only use it because conda-forge is the only repository where I can get data science libraries compiled for ARM64 macOS, and since I knew how to create environments there I just went with it. But yeah, this explains why it's not working. When I get time, I will make sure to try to understand the regular pip environments as well.

With regard to the release notes – you mean this file? Then I'll add it in the meantime.

@waylan
Copy link
Member

waylan commented Feb 9, 2021

With regard to the release notes – you mean this file? Then I'll add it in the meantime.

Yes, please add a line under the section "Other Changes and Additions to Version 1.2".

@nathanlesage
Copy link
Contributor Author

Got it, and committed.

@waylan
Copy link
Member

waylan commented Apr 4, 2021

Finally had the time to test this. This does not fix the issue. The last item is now completely hidden and the penultimate item appears to be the last item.

Here is a screenshot from before the fix:

Screen Shot 2021-04-04 at 3 36 44 PM

And here is a screenshot after the "fix" is applied:

Screen Shot 2021-04-04 at 3 35 40 PM

Note that the last item "License" is completely hidden. I am not able to scroll down any farther. As least previously it was accessible, albeit not fully exposed. Also, the "GitHub" and "next" links are not properly spaced apart anymore.

@nathanlesage
Copy link
Contributor Author

Thank you for testing it out, and I took that chance to try to set it up once again, and now I finally managed to install it in a way that my mkdocs-command is using the development version from the repository.

I was able to reproduce the bug you found and noticed that it was a wrong specificity in the ruleset (so that the base theme would override my changes).

I pushed the necessary changes; it now looks like this:


First page:

image

Any page in between:

image

Last page:

image


As you can see, now it should be fixed. I also took the liberty to move the repository link in between the Next/Previous-arrows. I figured this way it might look more symmetrical. If this is not desired, I'll undo this change.

@waylan
Copy link
Member

waylan commented Apr 5, 2021

I also took the liberty to move the repository link in between the Next/Previous-arrows. I figured this way it might look more symmetrical. If this is not desired, I'll undo this change.

Let's undo this.

@nathanlesage
Copy link
Contributor Author

Let's undo this.

Done.

@waylan
Copy link
Member

waylan commented Apr 9, 2021

Looks good. Thanks.

@waylan waylan merged commit eb31d4c into mkdocs:master Apr 9, 2021
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.

None yet

2 participants