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

Implements \mathchoice command #969

Merged
merged 8 commits into from Nov 22, 2017

Conversation

@konn
Copy link
Collaborator

commented Nov 16, 2017

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:

\newcommand{\infdisj}{%
  \mathop{%
    \mathchoice{% display
      \bigvee\hspace{-2ex}\bigvee%
    }{          % inline
      \bigvee\hspace{-1.75ex}\bigvee%
    }{          % script
      \bigvee\hspace{-1.4ex}\bigvee%
    }{          % scriptscript
      \bigvee\hspace{-1ex}\bigvee%
}}}

Screenshot test shows that there are still differences between LaTeX's and KaTeX's behaviour:
mathchoice
mathchoice2

I think it is a problem of the current implementations of \scriptstyle and \scriptscriptstyle in KaTeX, as there is already mismatch in StyleSpacing screenshot test:
stylespacing

konn added 2 commits Nov 16, 2017
@ronkok ronkok referenced this pull request Nov 17, 2017
@edemaine
Copy link
Member

left a comment

Nice idea to add this feature, and looks like a good implementation. Two small requests, assuming you agree.

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}}}

This comment has been minimized.

Copy link
@edemaine

edemaine Nov 21, 2017

Member

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.

This comment has been minimized.

Copy link
@konn

konn Nov 22, 2017

Author Collaborator

Sure. I've just combined them into one, and here is the result:

mathchoice

body = group.value.script;
} else if (style === Style.SCRIPTSCRIPT) {
body = group.value.scriptscript;
}

This comment has been minimized.

Copy link
@edemaine

edemaine Nov 21, 2017

Member

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.

This comment has been minimized.

Copy link
@konn

konn Nov 22, 2017

Author Collaborator

Exactly. I will define new chooseMathStyle function.

konn added 5 commits Nov 22, 2017
@edemaine
Copy link
Member

left a comment

Looks good! Could you remove the MathChoice2 images?

@konn

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2017

Oops, I forgot to remove them. Just deleted!

@edemaine
Copy link
Member

left a comment

Thanks!

@edemaine edemaine merged commit 6f1661f into master Nov 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@konn konn deleted the mathchoice branch Nov 22, 2017

@kevinbarabash

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

Cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.