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

Decouple the theme JS from readthedocs.org #3968

Merged
merged 12 commits into from
Jun 6, 2018

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Apr 18, 2018

This PR changes the JavaScript injected onto docs pages. It does not include the Read the Docs theme JS if it is already included. After readthedocs/sphinx_rtd_theme#614 (presumably sphinx_rtd_theme>0.3.0, edit >=0.4.0) it will use the version of JS specific to the theme version. This PR does not rely on the other and can be merged independently.

This PR also documents why certain fixes are necessary and the versions and themes they are necessary on.

Related to but does not rely on readthedocs/readthedocs-sphinx-ext#39.

One area for improvement is that we probably don't need to source the entire theme JS (it comes from Bower). Instead, we should separate the theme JS from the JS needed to power the version selector menu.

@davidfischer davidfischer requested a review from agjohnson April 18, 2018 16:16
@davidfischer
Copy link
Contributor Author

The lint errors are unrelated but there is an eslint error which is relevant. I believe I'll have to ignore it though since I specifically want to not have a global require. The point of this is to conditionally include JavaScript only for versions of the theme that require it.

The whole point is to conditionally require the theme JS
if it has not already been included.
@jessetan
Copy link
Contributor

Will this also include sphinx_rtd_theme if a different theme is used?

@davidfischer
Copy link
Contributor Author

Yes it will. That is how it currently works before this change as well.

The sphinx_rtd_theme JavaScript is used (even on other themes) to power the version selector menu. This change did not change that.

@jessetan
Copy link
Contributor

OK.

The sphinx_rtd_theme JavaScript is used (even on other themes) to power the version selector menu. This change did not change that.

This feels a bit uncleanly separated, but I don't have a good working suggestion at this time.

@davidfischer
Copy link
Contributor Author

This feels a bit uncleanly separated, but I don't have a good working suggestion at this time.

I don't disagree. This is also what I highlighted as an area for improvement above when I said:

One area for improvement is that we probably don't need to source the entire theme JS (it comes from Bower). Instead, we should separate the theme JS from the JS needed to power the version selector menu.

I just view that as a bigger change.

@agjohnson agjohnson added this to the 2.4 milestone Apr 30, 2018
@ericholscher
Copy link
Member

What's the state of this? Should we be moving forward w/ this now that 0.3.1 is out?

@davidfischer
Copy link
Contributor Author

What's the state of this? Should we be moving forward w/ this now that 0.3.1 is out?

I believe it should be rolled out.

@ericholscher
Copy link
Member

This is backwards compat with all versions of the theme right, so we can merge this, and it will work across all versions?

@davidfischer
Copy link
Contributor Author

This is backwards compat with all versions of the theme right, so we can merge this, and it will work across all versions?

It is supposed to, yes.

@agjohnson agjohnson modified the milestones: 2.4, 2.5 May 31, 2018
@davidfischer davidfischer merged commit 6bb7a2a 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.

4 participants