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

Always bundle the theme CSS/JS #614

Merged
merged 4 commits into from
May 31, 2018
Merged

Always bundle the theme CSS/JS #614

merged 4 commits into from
May 31, 2018

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Apr 18, 2018

This removes the {% if READTHEDOCS %} conditionals around the theme JS/CSS. The goal here is to uncouple the code injected by readthedocs.org into this theme from the version of the CSS/JS on the theme itself.

This change will have a required change in the readthedocs.org repo which I will link when that PR is created. There is also an optional (but highly recommended) change coming in the readthedocs-sphinx-ext repo which I will also link to.

@davidfischer
Copy link
Contributor Author

Here's the corresponding .org PR: readthedocs/readthedocs.org#3968

Without merging that PR, the theme JS will be included twice. This doesn't seem to break anything.

There's also a PR on the readthedocs-sphinx-ext repo: readthedocs/readthedocs-sphinx-ext#39

Without that PR, the theme CSS will be included twice. This doesn't break anything. One thing to note is that the CSS from media.readthedocs.org is included later in the DOM so it will take precedence over the theme bundled CSS. We should just merge the above PR to fix that.

@davidfischer
Copy link
Contributor Author

Coupled with the other PRs, this fixes #610.

Copy link
Contributor

@jessetan jessetan left a comment

Choose a reason for hiding this comment

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

LGTM. Would benefit from a changelog entry since the 0.3.0 theme is actually broken on RTD.

@davidfischer
Copy link
Contributor Author

Will do. I will also update the changelog to mention that 0.3.0 has some issues.

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 after fixing the changelog.

I wonder if we should release this as 1.0 -- since we should probably have had a 1.0 a while ago, this feels like a big enough change to justify it though. "RTD theme breaks free of RTD"

@ericholscher
Copy link
Member

Maybe we can remove the other 2 {% if READTHEDOCS %} sections, and have that be 1.0 :)

@davidfischer
Copy link
Contributor Author

Maybe we can remove the other 2 {% if READTHEDOCS %} sections, and have that be 1.0 :)

Let's make that a goal for 1.0. However, I think targeting this for 0.4.0 would be good.

@ericholscher ericholscher merged commit 53e2a78 into master May 31, 2018
@stsewd stsewd deleted the always-bundle-js-css branch January 2, 2019 23:39
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