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

Projects
None yet
4 participants
@davidfischer
Contributor

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 rtfd/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 rtfd/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 Apr 18, 2018

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Apr 18, 2018

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.

Ignore eslint global require
The whole point is to conditionally require the theme JS
if it has not already been included.
@jessetan

This comment has been minimized.

Contributor

jessetan commented Apr 20, 2018

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

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Apr 20, 2018

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

This comment has been minimized.

Contributor

jessetan commented Apr 23, 2018

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

This comment has been minimized.

Contributor

davidfischer commented Apr 23, 2018

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

This comment has been minimized.

Member

ericholscher commented May 24, 2018

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

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented May 31, 2018

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

This comment has been minimized.

Member

ericholscher commented May 31, 2018

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

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented May 31, 2018

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 and others added some commits Jun 6, 2018

Merge pull request #4199 from rtfd/fix-docs-lint
Fix Build: Convert md to rst in docs
Ignore eslint global require
The whole point is to conditionally require the theme JS
if it has not already been included.

@davidfischer davidfischer merged commit 6bb7a2a into rtfd:master Jun 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment