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

Exclude extra header from offline builders #63

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 27, 2019

Related to readthedocs/readthedocs.org#2637

We don't need to insert the extra header for offline docs.

I tested it locally, it works as expected. We are going to still see this footer

Screenshot_2019-03-27 Test Builds documentation

But all the links are dummy, and it's because we set the READTHEDOCS env variable, and we use that on the theme to put some dummy data before it gets replaced by the api response https://github.com/rtfd/sphinx_rtd_theme/blob/ddfdd35e6977f21fb1771734732756b0cf1d6bba/sphinx_rtd_theme/versions.html#L1

@humitos
Copy link
Member

humitos commented Mar 28, 2019

I'm not sure what this PR removes. "Downloads" header only or its content?

The screenshot shows the "Downloads" header with no links.

Also, it seems that we want to remove the "Versions" header and its content for the HTMLZip version since there is no another version for this format.

@stsewd
Copy link
Member Author

stsewd commented Mar 28, 2019

That comes from the theme https://github.com/rtfd/sphinx_rtd_theme/blob/ddfdd35e6977f21fb1771734732756b0cf1d6bba/sphinx_rtd_theme/versions.html#L1

Nothing we can do about that from the extension.

This removes the injected header

https://github.com/rtfd/readthedocs-sphinx-ext/blob/master/readthedocs_ext/_templates/readthedocs-insert.html.tmpl

Which points to some online resources and causes some errors in the js console. See readthedocs/readthedocs.org#2637

@ericholscher
Copy link
Member

Seems reasonable 👍

@ericholscher ericholscher merged commit ab5a7aa into readthedocs:master Jul 16, 2019
@ericholscher
Copy link
Member

Actually looking at the code, I don't fully understand this. Is there not a better test for this, like checking explicitly for htmlzip format? @stsewd

@ericholscher
Copy link
Member

Ignore me, I read it wrong :D

@stsewd
Copy link
Member Author

stsewd commented Jul 16, 2019

This PR was out of date, it should be using the constant

@ericholscher
Copy link
Member

I pushed up the proper variable name, and am testing locally

@ericholscher
Copy link
Member

@stsewd in testing now I'm getting Uncaught ReferenceError: READTHEDOCS_DATA is not defined

@ericholscher
Copy link
Member

Aaand, that's from the new Sphinx search extension -- it works without that :)

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

3 participants