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

Allow specifying URLs for LaTeX engines #1430

Merged
merged 12 commits into from
Oct 18, 2020
Merged

Allow specifying URLs for LaTeX engines #1430

merged 12 commits into from
Oct 18, 2020

Conversation

ianqsong
Copy link
Contributor

@ianqsong ianqsong commented Oct 4, 2020

add a field for MathJax CDN that user can input by calling Documenter.HTML

Edit by @mortenpi: fix #1428

@ianqsong ianqsong mentioned this pull request Oct 4, 2020
@mortenpi
Copy link
Member

mortenpi commented Oct 5, 2020

Thanks for the PR!

I think it would be more natural to handle all of it in the MathJax2/3 types. I.e. if you want to configure the URL, you'd just set something like HTML(mathengine = MathJax3(url="https://....")). No reason to add another global to the HTML struct. Beyond that:

  • Docstrings would need to be updated to reflect the new arguments.
  • It would be good to add a test to the example builds for overriding the URL, together with the relevant test in the tests file that would make sure that documenter.js contains the correct URL.

@ianqsong
Copy link
Contributor Author

ianqsong commented Oct 9, 2020

A new field url is added to MathJax2/3, of which the default value copies the original one, i.e., "cdnjs.cloudflare.com". So the tests file stays unchanged.

For such a small enhancement, I was wondering if one maintainer/committer could make it happen soon.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

I took the liberty of modifying the tests a bit, to make sure we test all the cases. I would have two more requests before merging though:

  1. Also add the same argument for KaTeX (for consistency).
  2. Update the docstrings to reflect this argument

src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
@mortenpi mortenpi changed the title add an arg. for mathjax cdn Allow specifying URLs for LaTeX engines Oct 12, 2020
test/examples/make.jl Outdated Show resolved Hide resolved
@ianqsong
Copy link
Contributor Author

ianqsong commented Oct 12, 2020

  1. Also add the same argument for KaTeX (for consistency).

I don't have much knowledge of KaTax, but there are three urls

.../katex.min.css
.../katex.min.js
.../auto-render.min.js

Would you confirm that this is also the case for other CDNs?

I was thinking it be more efficient if I hand over this PR to you and you could do this in a single commit -- what'd you say?

@mortenpi
Copy link
Member

I was thinking it be more efficient if I hand over this PR to you and you could do this in a single commit -- what'd you say?

I'm afraid that might not really be the case -- it's much less time-consuming just to review, so it would probably be a while before I get around to this in that situation.

Regarding KaTeX: you are right, I had forgotten that it was a bit more complicated. Let's leave KaTeX as it is.

@ianqsong
Copy link
Contributor Author

docstrings is updated. Many thanks for your help with the test cases.

@mortenpi mortenpi added this to the 0.25.3 milestone Oct 15, 2020
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Thanks @ianqsong! I took the liberty of doing a few edits. Once the tests pass, this should be good to go.

Actually, the query parameters are perfectly fine. We even use them in
the default MathJax v2 URL.

The confusion arose because the default URL

  https://cdn.jsdelivr.net/npm/mathjax@2/MathJax.js?config=TeX-AMS-MML_CHTML

is actually buggy -- there is no TeX-AMS-MML_CHTML, at least not in that
CDN for the 2.7.9 version of MathJax.
@ianqsong ianqsong closed this Oct 16, 2020
@mortenpi mortenpi reopened this Oct 16, 2020
@mortenpi
Copy link
Member

@ianqsong was the closing of the PR accidental?

@ianqsong
Copy link
Contributor Author

sorry, a fat-finger mistake

@mortenpi mortenpi merged commit f7c13a4 into JuliaDocs:master Oct 18, 2020
@ianqsong ianqsong deleted the mathjax-cdn-arg branch October 19, 2020 05:55
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.

script.src for MathJax3
2 participants