-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implements \mathchoice command #969
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I will define new chooseMathStyle
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Could you remove the MathChoice2 images?
Oops, I forgot to remove them. Just deleted! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Cool! |
This PR implements
\mathchoice
function.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
\scriptstyle
and\scriptscriptstyle
in KaTeX, as there is already mismatch inStyleSpacing
screenshot test: