Skip to content

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jun 13, 2018

Sphinx RTD theme uses a versions.html template to generate the version menu if READTHEDOCS variable is True. Since this is something specific to Read the Docs and that variable is going to be removed soon from the theme, we are overriding this template to maintain our current functionality.

Also, the static content generated in this menu is removed to avoid showing invalid links/data in case the AJAX fails for any reason. If this happen, the menu just won't show any information.

Related readthedocs/sphinx_rtd_theme#578

Sphinx RTD theme uses a `versions.html` template to generate the
version menu if `READTHEDOCS` variable is True. Since this is
something specific to Read the Docs and that variable is going to be
removed soon from the theme, we are overriding this template to
maintain our current functionality.

Also, the static content generated in this menu is removed to avoid
showing invalid links/data in case the AJAX fails for any reason. If
this happen, the menu just won't show any information.
@humitos humitos force-pushed the humitos/template/override-versions branch from 3110434 to 16ea4f4 Compare June 13, 2018 18:55
html_context = context

# Add custom RTD templates
if using_rtd_theme:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding the templates only when using our own theme, but what would happen with derivated themes?

They will have the content of the versions.html that comes from our sphinx_rtd_theme instead of this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. Maybe through some Sphinx internals we can determine if the theme in use extends our theme at some point? This sounds like a hard and error prone thing to determine though.

Ultimately, this might be a really good reason to keep this logic in our theme. The work required to replicate if READTHEDOCS is going to be substantial. Even if removing READTHEDOCS from our theme is technically correct, leaving it is the right thing to do.

@humitos
Copy link
Member Author

humitos commented Jun 13, 2018

This overrides works in the community and the corporate site without modification.

@humitos humitos requested a review from a team June 13, 2018 18:58
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation looks great, but I think you've raised another good reason to keep this logic in our theme.

@ericholscher
Copy link
Member

I don't think this is a reason to not have this logic live outside the RTD theme. I think a reasonable approach would be:

  • have the RTD theme use a block inside versions.html
  • Override this block in our version of the template

I think that gets everything we want, unless I'm mistaken?

@humitos
Copy link
Member Author

humitos commented Jun 14, 2018

@ericholscher replacing the {% if READTHEDOCS %} by {% block versions %} is enough for your proposal?

The only problem that I see there is that the default content of that block is only valid for Read the Docs, then the user from outside RTD will need to override the template and use an empty block.

Just to be clear, is people building docs with READTHEDOCS=True outside Read the Docs platform?

  • No: we could remove the whole content of versions.html file and replace it just by an empty block that we can override from the code of our sites
  • Yes: although it's weird, we need to keep the default content of the block in the theme template

What about the other way around of what I proposed in readthedocs/sphinx_rtd_theme#578? Instead of having a READTHEDOCS and the theme option we could have the theme option used to replace the {% if READTHEDOCS %} and use the block that you proposed for the inner part of the HTML (replacing the current {% if theme_versions_menu %}).

With the above proposal, we will have:

  • theme_versions_menu in False by default set in the theme: users outside RTD won't have a version menu rendered at all
  • theme_versions_menu as True under Read the Docs (using the same append_conf method than in this PR)
  • a template in Read the Docs to override the inner block with an empty block (in case we want to keep the default static block in the theme for some reason)

What do you think?

@ericholscher
Copy link
Member

@humitos why can't we just allow people building outside of RTD to use their own versions.html similar to what RTD is doing, with their own logic?

@ericholscher
Copy link
Member

I think maybe we should have a quick call about this to discuss. I think there are a number of competing priorities here, and it might be easier this way :)

@agjohnson agjohnson added the Needed: design decision A core team decision is required label Jun 14, 2018
else:
html_context = context

# Add custom RTD templates
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were talking to move this code into the extra_context block and use it in the corporate site.

@humitos
Copy link
Member Author

humitos commented Jan 9, 2019

I'm closing this as stale. I suppose that before we find a good way to do this we will end up having APIv3 with a flyout rendered via a JSON response.

@humitos humitos closed this Jan 9, 2019
@stsewd stsewd deleted the humitos/template/override-versions branch May 11, 2020 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needed: design decision A core team decision is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants