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

Stop injecting the theme CSS on the RTD theme #39

Merged

Conversation

davidfischer
Copy link
Contributor

This performs a version check on the Read the Docs theme and only injects the theme.css for older versions of the theme. The goal here is to break the tight coupling between the bundled code on readthedocs.org and the theme.css. Previously, the theme did not include the CSS if it was being built on Read the Docs. With this change, the theme CSS can be updated independently of Read the Docs.

Related to readthedocs/sphinx_rtd_theme#614 but that PR does not strictly require this one. Without this change, both CSS files (a local one and one from readthedocs.org) will be included on theme versions >0.3.0.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good. I'm a big +1 on making the RTD theme just another theme, and we don't treat it special (or have it treat us specially).


# After v0.3.0 of the sphinx theme, the theme CSS should not be injected
# This decouples the theme CSS (which is versioned independently) from readthedocs.org
if theme_css.endswith('sphinx_rtd_theme.css'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's safer to check if context['html_theme'] == 'sphinx_rtd_theme'. The current check does not work if app.builder.name == 'readthedocssinglehtmllocalmedia' (see line 60).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing is necessary with readthedocssinglehtmllocalmedia and this change purposely doesn't apply there. That builder doesn't use the theme CSS on media.readthedocs.org.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alrighty 👍

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks like a solid change.

if theme_css.endswith('sphinx_rtd_theme.css'):
try:
import sphinx_rtd_theme
inject_css = LooseVersion(sphinx_rtd_theme.__version__) <= LooseVersion('0.3.0')
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to update the theme version here, or will this work with 0.3.0 and 0.3.1, and this logic can stay?

Copy link
Member

Choose a reason for hiding this comment

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

Believe it depends on readthedocs/sphinx_rtd_theme#614 being released right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Believe it depends on readthedocs/sphinx_rtd_theme#614 being released right?

Correct so perhaps we should bump to 0.4.0.

@davidfischer
Copy link
Contributor Author

I updated this PR to stop injecting the theme CSS starting at v0.4.0 of the them (which isn't released)

@davidfischer davidfischer merged commit e19546f into readthedocs:master Jun 6, 2018
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.

3 participants