Conversation
edemaine
left a comment
There was a problem hiding this comment.
Nice idea to add this feature, and looks like a good implementation. Two small requests, assuming you agree.
test/screenshotter/ss_data.yaml
Outdated
| MathChoice: | | ||
| {\displaystyle\mathchoice{D}{T}{S}{SS}} {\textstyle\mathchoice{D}{T}{S}{SS}} {\scriptstyle \mathchoice{D}{T}{S}{SS}} {\scriptscriptstyle\mathchoice{D}{T}{S}{SS}} | ||
| MathChoice2: | | ||
| \displaystyle X_{\mathchoice{D}{T}{S}{SS}_{\mathchoice{D}{T}{S}{SS}}} |
There was a problem hiding this comment.
Can you merge these two tests (e.g. side-by-side)? I think we try to keep down the number of snapshot images, and these tests render small enough (and they're related) that it's easy to combine them.
There was a problem hiding this comment.
Sure. I've just combined them into one, and here is the result:
src/functions/mathchoice.js
Outdated
| body = group.value.script; | ||
| } else if (style === Style.SCRIPTSCRIPT) { | ||
| body = group.value.scriptscript; | ||
| } |
There was a problem hiding this comment.
Given that these lines (44-54) are repeated (25-35), perhaps it makes sense to factor them out as a shared function defined just within and for this module? Not strictly necessary, but could make for cleaner code.
There was a problem hiding this comment.
Exactly. I will define new chooseMathStyle function.
edemaine
left a comment
There was a problem hiding this comment.
Looks good! Could you remove the MathChoice2 images?
|
Oops, I forgot to remove them. Just deleted! |
|
Cool! |
This PR implements
\mathchoicefunction.I once created PR on the wrong branch. Sorry for the mess.
This is particularly useful when one defines custom macro for "big operators".
For example:
Screenshot test shows that there are still differences between LaTeX's and KaTeX's behaviour:
I think it is a problem of the current implementations of
\scriptstyleand\scriptscriptstylein KaTeX, as there is already mismatch inStyleSpacingscreenshot test: