-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Adds testing and hopefully a final fix for the empty toctree issue #375
Conversation
This adds some more hacky fixes to our layout to avoid singlehtml builders. Instead of trying to guess if this is working, I added testing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Hopefully snide will be moving the theme over to @rtfd tonight, and then we can setup Travis to confirm.
sphinx_rtd_theme/layout.html
Outdated
@@ -129,7 +129,7 @@ | |||
The singlehtml builder doesn't handle this toctree call when the | |||
toctree is empty. Skip building this for now. | |||
#} | |||
{% if builder != 'singlehtml' %} | |||
{% if builder not in ['singlehtml', 'readthedocssinglehtml', 'readthedocssinglehtmllocalmedia'] %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to do if 'singlehtml' in builder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated it. It's probably a cleaner implementation, so 👍
'<a class="reference internal" href="index.html#document-bar">bar</a>' | ||
'</li>\n' | ||
'</ul>' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems brittle, I wonder if we could just look for the class names? local-toc
vs. toctree-wrapper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be all that brittle, but I do think it's important to test the full thing. Otherwise, we won't be testing if the toctrees have anything in them, which is the more important part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. 👍
Ha, just happened :) |
This adds some more hacky fixes to our layout to avoid singlehtml builders.
Instead of trying to guess if this is working, I added testing!