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

Move language-specific data into a new JS file, language_data.js #5590

Merged
merged 1 commit into from
Nov 11, 2018

Conversation

mitya57
Copy link
Contributor

@mitya57 mitya57 commented Nov 4, 2018

Subject: Make search work with old templates

Feature or Bugfix

Bugfix

Purpose

Not all templates use documentation_options.js. The examples are old versions of sphinx-rtd-theme and many other third-party themes. However currently searchtools.js does not work without some classes, which are defined in documentation_options.js. This pull request moves these classes to a separate file language_data.js which is always included, even for old templates.

Relates

Fixes #5460.

This file is included in script_files, so it will be present even for
projects using custom templates.

Fixes sphinx-doc#5460.
@codecov
Copy link

codecov bot commented Nov 4, 2018

Codecov Report

Merging #5590 into 1.8 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.8    #5590      +/-   ##
==========================================
+ Coverage   82.05%   82.06%   +<.01%     
==========================================
  Files         306      306              
  Lines       40390    40391       +1     
  Branches     6241     6241              
==========================================
+ Hits        33144    33145       +1     
  Misses       5862     5862              
  Partials     1384     1384
Impacted Files Coverage Δ
sphinx/builders/html.py 82.95% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6686711...2a9cad7. Read the comment docs.

Copy link
Member

@TimKam TimKam left a comment

Choose a reason for hiding this comment

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

I think this looks good and can confirm it fixes the issue with the ReadTheDocs template. Still, I wonder if it makes more sense to include documentation_options.js_t via self.add_js_file() and to fix everything in documentation_options.js_t. What do you think, @tk0miya?

@mitya57
Copy link
Contributor Author

mitya57 commented Nov 9, 2018

@TimKam it is difficult to load it via self.add_js_file() because it needs the data-url_root attribute. I am not sure I understand fully why that attribute is used/needed, so I implemented a solution that is easier.

@TimKam
Copy link
Member

TimKam commented Nov 9, 2018

Thanks for clarifying. Then I think there is no trivial way to improve this. I'm fine with this PR :-)

@tk0miya tk0miya added this to the 1.8.3 milestone Nov 11, 2018
@tk0miya
Copy link
Member

tk0miya commented Nov 11, 2018

+1.

I just released 1.8.2 without merging this because of overlooking. So I'm merging this for 1.8.3.

@tk0miya tk0miya merged commit f9f52ab into sphinx-doc:1.8 Nov 11, 2018
tk0miya added a commit that referenced this pull request Nov 11, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants