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
Merged

Implements \mathchoice command #969

merged 8 commits into from
Nov 22, 2017

Conversation

konn
Copy link
Collaborator

@konn konn 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

@ronkok ronkok mentioned this pull request Nov 17, 2017
Copy link
Member

@edemaine edemaine left a 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.

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}}}
Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

@edemaine edemaine left a 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?

@konn
Copy link
Collaborator Author

konn commented Nov 22, 2017

Oops, I forgot to remove them. Just deleted!

Copy link
Member

@edemaine edemaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@edemaine edemaine merged commit 6f1661f into master Nov 22, 2017
@konn konn deleted the mathchoice branch November 22, 2017 12:36
@kevinbarabash
Copy link
Member

Cool!

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.

3 participants