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

Default HTML tags #1

Closed
rowanc1 opened this issue Jul 5, 2021 · 6 comments · Fixed by #4
Closed

Default HTML tags #1

rowanc1 opened this issue Jul 5, 2021 · 6 comments · Fixed by #4
Labels
enhancement New feature or request

Comments

@rowanc1
Copy link
Member

rowanc1 commented Jul 5, 2021

Is your feature request related to a problem? Please describe.

Looking at the default renderer here: https://github.com/executablebooks/markdown-it-amsmath/blob/master/src/index.ts#L45

It is currently <section class="amsmath"><eqn>{content}</eqn></section>.

Reading the HTML spec, sections require a header in most situations and generally have more content than a single element. Also note that eqn is not a valid tag and I don;t think we should be providing this as the default.

Describe the solution you'd like

I would propose we use a div with a class for the default.

Either:

<div class="amsmath">{content}</div>

Or simply:

<div class="math amsmath">{content}</div>

Which might work better with dollarmath (math inline and math display) and keep css classes simple/shared.

Describe alternatives you've considered

This can be overridden through the renderer, however, the default should provide good basics and valid html - sticking to semantic rules if possible!

Additional context

In the previous js implementation of this, we were adding a \[ before and after the div, this allowed the default script of mathjax and katex to render when this HTML was output. The class chosen was math rather than amsmath because from a rendering perspective there isn't a difference, so class selection seemed easier with that choice.

@rowanc1 rowanc1 added the enhancement New feature or request label Jul 5, 2021
@welcome
Copy link

welcome bot commented Jul 5, 2021

Thanks for opening your first issue here! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out EBP's Code of Conduct. Also, please try to follow the issue template as it helps other community members to contribute more effectively.

If your issue is a feature request, others may react to it, to raise its prominence (see Feature Voting).

Welcome to the EBP community! 🎉

@chrisjsewell
Copy link
Member

chrisjsewell commented Jul 5, 2021

Happy to discuss, but just to give my initial thoughts:

This can be overridden through the renderer, however, the default should provide good basics and valid html

IMO the default renderer is literally only there for testing purposes, it should never actually be used in production, i.e. you should always set a renderer.
This is why TBH, when I created https://github.com/executablebooks/mdit-py-plugins/tree/master/mdit_py_plugins/amsmath, I really did not put too much thought into it, except the fact that I wanted to capture exactly what had been passed in the render, so that the tests were good

In the previous js implementation of this, we were adding a \[ before and after the div, this allowed the default script of mathjax and katex to render when this HTML was output.

I very much don't think this is how this package should be used. In sphinx we have to have mathjax search for the math, because (the javascript implemented) mathjax can not be accessed via the Python API. There are issues around this, e.g.:

  • Its slow because mathjax has to walk through the whole HTML syntax tree to find math, that we have already identified via markdown-it
  • It can potentially mis-intepret text as math, if e.g. the content was backslash esaped then markdown-it removes those escapes

with this package, we have "proper" API access to these packages, and do not need to go through an intermediary.

@chrisjsewell
Copy link
Member

chrisjsewell commented Jul 5, 2021

note as well any changes here would also have to mirrored in https://github.com/executablebooks/mdit-py-plugins/tree/master/mdit_py_plugins/amsmath

@rowanc1
Copy link
Member Author

rowanc1 commented Jul 6, 2021

It seems like if we tweak the basic template and update the tests, then this will be fine to use in production (the render later approach) - and that seems like a better test suite anyways and not much work. I will add a few other tests that make use of the HTML sanitizing.

Agree on the \[. Makes enough sense to loop on known nodes rather than a text lookup. It was me using the default mathjax/katex renderer.

Will start a PR here and then open one on the python repo.

@chrisjsewell
Copy link
Member

cool cheers, yep definitely good for you to do the Python one as well 👍 I certainly fee it is key to keep the python/typescript implementations in-sync, so we can work towards this "language agnostic" specification

@rowanc1
Copy link
Member Author

rowanc1 commented Jul 6, 2021

Awesome - I feel like tracing some of these small issues/improvements all the way through will also help me get my bearings in the python stack, and get a better feeling for how everything on that side hangs together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants