jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
177 stars 28 forks source link

add mermaid.js and mathjax to npm deps #231

Closed 2bndy5 closed 1 year ago

2bndy5 commented 1 year ago

resolves #150

jbms commented 1 year ago

I think rather than download mermaid.js when the docs are built, we could just add it as a dependency to packages.json and then include it in the built package / wheel with the other javascript bundles (but not bundle it). We can do the same thing for mathjax.

2bndy5 commented 1 year ago

I had considered that as well, but I wasn't sure if changing the package.json was a welcome change. I'll rework this and get back to you...

I'm still not convinced that the mathjax lib needs to be handled by this theme. Sphinx' mathjax_path can be used by users to do the same thing that was suggested. Maybe I'm missing something.

jbms commented 1 year ago

It is true that mathjax isn't really related to this theme. Perhaps it would be better if Sphinx itself included a version of mathjax so that there is no dependency on external resources by default. But I'm not sure how likely that is to go through...

Particularly since we may soon provide a way to use mathml instead, I think it is reasonable to just ignore mathjax for now...

But in general providing a way to avoid any dependence on external resources is a useful feature, and one that can be a bit annoying for users to implement themselves.

2bndy5 commented 1 year ago

Perhaps it would be better if Sphinx itself included a version of mathjax so that there is no dependency on external resources by default

Oh, you're focused on the default behavior. 🤦🏼 That would be so easy to just change the default value of mathjax_path if the config has the attr defined... I'm not sure where we'd document this adjustment. Maybe it can be mentioned with the solution for #233 .

jbms commented 1 year ago

Regarding mathjax, now that all major browsers support MathML, it may make sense to explore whether we can just recommend using mathml instead of mathjax, since if it works, it would require no additional libraries and would render without a delay.

This comment describes how it might simply require modifying the docutils.conf file in the doc root directory:

https://github.com/sphinx-doc/sphinx/issues/6092#issuecomment-1501088319

Not sure if anyone has actually tested that, though.

2bndy5 commented 1 year ago

I was going to approach the #233 separately. The newer MathML feature does look much more appealing, but this work is more like retrofitting when it comes to mathjax use. The sphinx dev cycle isn't too slow (for the most part), so hopefully there will be more development/testing with MathML in Sphinx by the time we get around to #233 .

jbms commented 1 year ago

Can you also ensure that the licenses for these two packages are added to the generated license file (see tools/buil.d/transform/index.ts),; the existing logic only includes packages that are bundled by esbuild.

2bndy5 commented 1 year ago

Can you also ensure that the licenses for these two packages are added to the generated license file

The mermaid dist includes a copy of the License alongside the mermaid.min.js* files. Did you want to keep that in the bundles folder as well? Or is it sufficient to have it only in the generated LICENSE file?

jbms commented 1 year ago

Can you also ensure that the licenses for these two packages are added to the generated license file

The mermaid dist includes a copy of the License alongside the mermaid.min.js* files. Did you want to keep that in the bundles folder as well? Or is it sufficient to have it only in the generated LICENSE file?

Keeping a copy of the license there as well makes sense.