-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Remove static footer content #578
Conversation
I tested by myself manually, but probably this will need more eyes for this since it's a tricky edge case. |
Hmm... Currently, it is possible for people not on RTD to use this to add versioning support themselves (with the help of #574) See also #543 I think this is something we should support. Also, I cannot open that link, I assume we need special permissions or something? So not sure if this is actually fixing anything. |
Oh, I see that you added a Anyway, I don't understand how the use of this theme outside RTD would allow the user to have multiple versions.
Yeah, it's the private repo of readthedocs.com. Basically, the problem is that the AJAX request fails for any reason, the footer is not overriden and the one that is generated by this template is invalid and cause problems. So, basically, when running on RTD there is no need to create that footer statically. In the PR that you pointed me, we will still have the problem I'm trying to fix. So, we need to find a different solution, but I need to understand how the user will use multiples versions first, I guess. |
If people manually add the |
You can have a look at https://github.com/buildthedocs/ and https://github.com/buildthedocs/btd. The build procedure itself is a 300 line shell script: https://github.com/buildthedocs/btd/blob/master/btd/build.sh It's just a proof-of-concept (yet), so it is far from being ready for production. However, there a couple of sites which are already built with it:
Note that I did some 'minor' visual modifications, which make the theme look slightly different, but the plain rtd theme would also work: https://github.com/buildthedocs/sphinx_btd_theme/tree/add-versioning. These modifications are:
In the end, buildthedocs is just a shell version of what I thought that readthedocs-build was meant for. See readthedocs/readthedocs-build#35. Hint: in order to pass context info from shell script(s) to python, a json file file is used. buildthedocs generates a context.json file in the folder where conf.py is located. Then, you need to add this snippet to the config file: https://github.com/buildthedocs/btd/blob/master/doc/conf.py#L141-L145 |
Since this is a rather big change I purpose we wait till after |
As raised on another issue, we don't want to support build tools that aren't Read the Docs with our theme, maintenance across our repositories is already spread thin and this complicates things more. Because we need to remove this block on readthedocs.com and readthedocs.org, there are only two cases we should support here:
Eventually the menu is to going to be reworked entirely and I'm not yet sure where this lives. I think it makes sense to maintain the menu styling inside RTD and at that point, the menu becomes a generalized feature specific to RTD, not our Sphinx theme. We're still shaping thoughts here though, so this is a future design decision. For now, it probably makes sense to wrap the menu in conditional logic so that it's off by default on RTD. This preserves local development. |
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.
Surround this in a conditional block, perhaps even a named block.
609144c
to
ea6f59e
Compare
@agjohnson I'm not 100% sure if this way of wrapping that static menu is the correct. Please, let me know. (I explained my idea in the commit message on how it should work) |
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.
LGTM if this is the way we are going to go on RTD.
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 currently breaks the local dev for the menu. We'll need a method that allows for local dev on the menu here for now (as we haven't begun discussing stylizing the menu elsewhere) and allows for disabling on read the docs.
In readthedocs.com/.org is not needed the usage of the bottom left versions menu, so we are making this a theme config to control it from outside. The default value is to add the menu (too keep backward compatibility).
1b224cb
to
dfc569a
Compare
I make this We will need to define it under Please, let me know if this is correct since "I have no idea what I'm doing" :) |
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 like this approach much better. It's backwards compat, and lets us configure it the way we want. 👍
sphinx_rtd_theme/versions.html
Outdated
@@ -7,6 +7,7 @@ | |||
<span class="fa fa-caret-down"></span> | |||
</span> | |||
<div class="rst-other-versions"> | |||
{% if theme_versions_menu %} |
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 wonder if this should go above, on the include versions.html
part? Do we really want all the above HTML in the theme around versions when there isn't a version menu to display?
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.
We still need this for the ajax call though.
sphinx_rtd_theme/versions.html
Outdated
@@ -7,6 +7,7 @@ | |||
<span class="fa fa-caret-down"></span> | |||
</span> | |||
<div class="rst-other-versions"> | |||
{% if theme_versions_menu %} |
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.
Clean up the whitespace with {%- if theme_versions_menu %}
else we get 20 blank lines if this is false
http://jinja.pocoo.org/docs/2.10/templates/#whitespace-control
sphinx_rtd_theme/versions.html
Outdated
@@ -7,6 +7,7 @@ | |||
<span class="fa fa-caret-down"></span> | |||
</span> | |||
<div class="rst-other-versions"> | |||
{% if theme_versions_menu %} |
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.
You should ask explicitly if this true and cast it to bool.
Is this PR something we are still interested in merging or should we close it? |
I think this was because of the footer not working with the authentication system in rtd. But now we don't have that problem. And actually we do use this as backup when we are on high load, and we disable the footer for a while. Also, I think a better way would be to just check if there are versions, if users don't want to put versions they simple just set |
Closing in favor of #1107 |
This content is injected by an AJAX call to the server.
Ref: https://github.com/readthedocs/readthedocs-corporate/issues/208