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

Add explicit dependency to sphinx-rtd-theme (and through it, jQuery) #239

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

rbberger
Copy link
Collaborator

@rbberger rbberger commented Mar 16, 2023

PR Summary

Newer Sphinx and sphinx-rtd-theme do not include jQuery, which is a necessary feature for sphinx_multiversion to properly work. This adds the sphinx_rtd_theme extension to our doc configuration, which also pulls in sphinxcontrib-jquery for us.

See https://blog.readthedocs.com/sphinx6-upgrade/

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • If preparing for a new release, update the version in cmake.

@rbberger rbberger marked this pull request as ready for review March 16, 2023 18:31
@rbberger rbberger requested a review from jonahm-LANL March 16, 2023 18:32
@rbberger rbberger added the documentation Improvements or additions to documentation label Mar 16, 2023
@rbberger rbberger force-pushed the rberger_fixup_jquery branch from ac05d2f to fd99e45 Compare March 16, 2023 18:35
@rbberger rbberger changed the title Draft: Add explicit dependency to jQuery Draft: Add explicit dependency to sphinx-rtd-theme (and through it, jQuery) Mar 16, 2023
@rbberger rbberger changed the title Draft: Add explicit dependency to sphinx-rtd-theme (and through it, jQuery) Add explicit dependency to sphinx-rtd-theme (and through it, jQuery) Mar 16, 2023
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

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

Thanks @rbberger for fixing this. I'll wait for @Yurlungur but I think this is a pretty straightforward change

Tagging @ktsai7 in case she has comments

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @rbberger !

@Yurlungur
Copy link
Collaborator

We should add a changelog otherwise I approve. Merge when ready (after tests pass).

@ktsai7
Copy link
Contributor

ktsai7 commented Mar 17, 2023

just out of curiosity, since this is a fix, what was broken?

@rbberger
Copy link
Collaborator Author

rbberger commented Mar 17, 2023

@ktsai7 Newer versions of Sphinx and the rtd theme no longer include an embedded jQuery. Instead it now must be provided with the sphinxcontrib-jquery extension. rtd pulls that in for you, but for that you have to properly add it as extension to your config. Without jQuery, dynamic layout elements such as the version selection with multiversion fail due to missing JavaScript.

@Yurlungur
Copy link
Collaborator

I added the changelog and will merge when tests pass.

@jhp-lanl jhp-lanl merged commit 0440ff3 into main Mar 17, 2023
@jhp-lanl jhp-lanl deleted the rberger_fixup_jquery branch March 17, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants