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

[draft] Remove option mathjax_classes #406

Conversation

cpitclaudel
Copy link
Contributor

@cpitclaudel cpitclaudel commented Jul 19, 2021

The global approach in #395 isn't really sustainable: it requires all-ways cooperation between all projects that want to customize MathJax. Additionally, when processing a MyST document without Sphinx, the MathJax configuration changes are not performed (part of #347). And, of course, this approach of overriding the MathJax object causes issues down the line for projects that need to customize MathJax (the setting in Sphinx isn't sufficient, see sphinx-doc/sphinx#9450)

@chrisjsewell you wrote:

Its not really ideal that either package has to override the (global) configuration

I think the following two approaches would not require overriding the global config:

  1. Add a custom script instead of touching the mathjax3_config variable; something like this, essentially:

    app.add_js_file(None, priority=0, body="""
       var MathJax = window.MathJax || MathJax;
       MathJax.options = MathJax.options || {};
       MathJax.options.processHtmlClass = (MathJax.options.processHtmlClass || "")
       + "|math";
    """)
  2. Don't touch MathJax_config at all; instead, add an explicit mathjax_process class on all math nodes, either by changing docutils_renderer (this PR) or by adding a Docutils transform to processes all math nodes:

class ActivateMathJaxTransform(Transform):
    default_priority = 800

    @staticmethod
    def is_math(node):
        return isinstance(node, (math, math_block))

    def apply(self, **kwargs):
        for node in self.document.traverse(self.is_math):
            node.attributes.setdefault("classes", []).append("mathjax_process")

This PR isn't ready for merging; it's just to start a discussion.

The global approach in 395 isn't really sustainable: it requires all-ways
cooperation between all projects that want to customize MathJax.  Additionally,
when processing a MyST document without Sphinx, the MathJax configuration
changes are not performed (part of executablebooks#347).  And, of course, this approach of
overriding the MathJax object causes issues down the line for projects that need
to customize MathJax (the setting in Sphinx isn't sufficient, see sphinx-doc/sphinx#9450)

The following two approaches would not cause these issues:

1. Add a custom script instead of touching the mathjax3_config variable;
   something like this, essentially:

   ```js
   app.add_js_file(None, priority=0, body="""
      var MathJax = window.MathJax || MathJax;
      MathJax.options = MathJax.options || {};
      MathJax.options.processHtmlClass = (MathJax.options.processHtmlClass || "")
      + "|math";
   """)
   ```

- Don't touch MathJax_config at all; instead, add an explicit `mathjax_process`
  class on all math nodes, either by changing `docutils_renderer` (this PR) or by
  adding a Docutils transform to processes all math nodes:

  ```python
  class ActivateMathJaxTransform(Transform):
      default_priority = 800

      @staticmethod
      def is_math(node):
          return isinstance(node, (math, math_block))

      def apply(self, **kwargs):
          for node in self.document.traverse(self.is_math):
              node.attributes.setdefault("classes", []).append("mathjax_process")
  ```

This PR isn't ready for merging; it's just to start a discussion.
@welcome
Copy link

welcome bot commented Jul 19, 2021

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! 🎉

@chrisjsewell
Copy link
Member

Don't touch MathJax_config at all; instead, add an explicit mathjax_process class on all math nodes, either by changing docutils_renderer (this PR)

Yeh but this would break any other extension that adds math and likely has not added explicitly the mathjax_process class. Therefore, I won;t be excepting this in its current form. Unfortunately docutils nor sphinx have really provided any good mechanism to deal with this

@chrisjsewell chrisjsewell marked this pull request as draft August 22, 2021 07:30
@cpitclaudel
Copy link
Contributor Author

I don't understand — isn't it the reverse? What MyST currently does breaks extensions that use math.

@chrisjsewell
Copy link
Member

chrisjsewell commented Aug 22, 2021

No because the configuration sets mathjax to run on all elements with the math class, which is added for all math and math_blocks

@cpitclaudel
Copy link
Contributor Author

Unless, of course, some other extensions modifies the Mathjax config (which is the issue at hand); and unless the document is processed with Docutils instead of Sphinx, too. That's why I was suggesting to add mathjax_process on relevant nodes. But it's probably OK to leave this unaddressed.

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.

None yet

2 participants