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

FIX: Enable maths in singlehtml mode #1774

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

SimplyOm
Copy link
Contributor

@SimplyOm SimplyOm commented Jul 6, 2022

Setting the html_assets_policy flag to always includes mathjax in singlehtml builder. This also helps in maths rendering for pdfhtml builder. This flag is defaulted to per_page inside sphinx codebase, which leads to excluding mathjax for singlehtml builder.

Resolves #1705

Setting the html_assets_policy flag  to always installs mathjax in singlehtml builder. This also helps in maths rendering for pdfhtml builder.
@welcome
Copy link

welcome bot commented Jul 6, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #1774 (7304e94) into master (4f0944f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
+ Coverage   91.37%   91.39%   +0.02%     
==========================================
  Files           7        7              
  Lines         684      686       +2     
==========================================
+ Hits          625      627       +2     
  Misses         59       59              
Flag Coverage Δ
pytests 91.39% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jupyter_book/sphinx.py 88.00% <100.00%> (+0.32%) ⬆️

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 4f0944f...7304e94. Read the comment docs.

@AakashGfude
Copy link
Contributor

AakashGfude commented Jul 6, 2022

Thanks, @SimplyOm for the PR.
@choldgraf what do you think of this change. Do you think there will be any reason to use html_assets_policy = "per_page" for single HTML builds? I think "always" is a good option for consistency even though it might lead to the loading of some redundant assets.

@SimplyOm
Copy link
Contributor Author

SimplyOm commented Jul 8, 2022

Tagging @choldgraf here just in case he might have missed the previous comment. Please let us know how you think about this change.

@SimplyOm
Copy link
Contributor Author

Hi @AakashGfude - not sure if @choldgraf is looking into this issue. Do you see any other concern approving this?

An alternate choice can be to add this as a parameter to the config, defaulted to false. Then, if someone is okay adding assets to singlehtml, they can turn it on.

@AakashGfude
Copy link
Contributor

Hi @AakashGfude - not sure if @choldgraf is looking into this issue. Do you see any other concern approving this?

An alternate choice can be to add this as a parameter to the config, defaulted to false. Then, if someone is okay adding assets to singlehtml, they can turn it on.

Hi @SimplyOm, I think @choldgraf is a bit occupied at the moment, he will get back soon.
I feel like this setting will always be good to have unless someone has a lot of assets which is slowing down the loading of the page. I am not sure how often that case is, but have a hunch that it won't be very often. So, I am okay with not making it a setting.

@mmcky
Copy link
Contributor

mmcky commented Jul 14, 2022

@AakashGfude and @SimplyOm I think having it enabled by default on a per page basis is a good idea. It will resolve the case where sphinx doesn't add math due to the lack of a math directive. I am in favour of this.

My only thought is sometimes the time it takes mathjax to render, the HTML scrapper takes a snapshot of the page prior to math being fully rendered. I think a small wait time has already been added previously?

@mmcky mmcky self-requested a review July 14, 2022 00:54
@AakashGfude
Copy link
Contributor

My only thought is sometimes the time it takes mathjax to render, the HTML scrapper takes a snapshot of the page prior to math being fully rendered. I think a small wait time has already been added previously?

Good point @mmcky, not entirely sure about the wait time being added thing. Thank you for approving this, will merge this for now. And we can further enhance this in subsequent PRs?

@AakashGfude AakashGfude merged commit e4c001b into jupyter-book:master Jul 14, 2022
@welcome
Copy link

welcome bot commented Jul 14, 2022

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@choldgraf
Copy link
Collaborator

It looks good to me - please don't feel like I need to approve before merging PRs. If you think it's a good change just go for it 🙂

@SimplyOm
Copy link
Contributor Author

Thank you all for the help! Much appreciated.

@SimplyOm SimplyOm deleted the SimplyOm-patch-pdfhtml branch July 14, 2022 06:43
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.

Math not rendering on pdf using pdfhtml builder
4 participants