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 the alignedat environment for flexible math spacing #930

Merged
merged 13 commits into from
Nov 12, 2017

Conversation

konn
Copy link
Collaborator

@konn konn commented Oct 12, 2017

Implemented alginat and alignat* environments from amsmath package.
They are display math environments, similar to align and align* environments, but adds no spacing commands such as \qquad between maths, which permits users to adjust spacing manually.

Use cases:

\begin{alignat*}{2} % alignat requires number of maths per row as an argument.
  1 &= 2 &\quad &(\because \bot)\\
  xyz &= zyx && (\text{brabra})
\end{alignat*}

\begin{alignat*}{3}
  x &= 2 &\quad zzz &< 5 & \hspace{1cm} \int_{-\infty}^\infty e^{-x^2} \mathrm{d}x &= \mathord{???}\\
  z - 5 &= w + q && &\hspace{1cm} 1+2+3+4&< 20
\end{alignat*}

With LaTeX, we get:

latex-image-2

@khanbot
Copy link

khanbot commented Oct 12, 2017

CLA signature looks good 👍

@edemaine
Copy link
Member

Given how similar aligned and alignat are, do you think the code could be shared between the two? You can list multiple functions in defineFunction, then branch within the code (adding space only when in aligned mode, say).

Another question: if I understand correctly, alignat can appear only in text mode, whereas alignedat is the equivalent within math mode. If this is correct, I think you are implementing the latter (in the same way that KaTeX currently supports aligned but not align) and it should be renamed accordingly.

@konn
Copy link
Collaborator Author

konn commented Oct 12, 2017

@edemaine Thank you for your rapid reply!

Given how similar aligned and alignat are, do you think the code could be shared between the two? You can list multiple functions in defineFunction, then branch within the code (adding space only when in aligned mode, say).

I think they can share code. I will try.

Another question: if I understand correctly, alignat can appear only in text mode, whereas alignedat is the equivalent within math mode. If this is correct, I think you are implementing the latter (in the same way that KaTeX currently supports aligned but not align) and it should be renamed accordingly.

Ah, I overlooked alignedat environment. Yes, this should definitely be that.

Anyway, I will resolve these soon. Thanks!

@konn
Copy link
Collaborator Author

konn commented Oct 12, 2017

I've just renamed alignat to alignedat and factored out duplicated code.
I couldn't find the way to vary the numArgs in one defineEnvrionment block, so I create new local function alignedHandler for that purpose.

@kevinbarabash
Copy link
Member

Cool! @konn would you be able to add a screenshot test for this new environment?

@konn
Copy link
Collaborator Author

konn commented Oct 13, 2017

@kevinbarabash OK, I've just added a code fragment for screenshot testing. Unfortunately, I couldn't run docker image appropriately and couldn't get the diff.

@kevinbarabash
Copy link
Member

@konn yeah getting docker set up is a bit of work. I can run the tests and send you a PR with the images.

@konn
Copy link
Collaborator Author

konn commented Oct 17, 2017

OK, finally here comes the screenshot diff of alignedats:

alignedat

It seems that KaTeX renders alignedat appropriately.

By the way, it seems that KaTeX fails to render aligned environment correctly, even before this pull req is applied:

aligned

Should we fix this in a separate pull request, or within this pull req?

@kevinbarabash
Copy link
Member

It seems that KaTeX renders alignedat appropriately.

The screenshots look great.

Should we fix this in a separate pull request, or within this?

Thanks for checking the output against LaTeX's. Since it's an existing issue a separate PR is appropriate. Can you open a new issue for this?

@kevinbarabash
Copy link
Member

@konn I'm going try to review this later this week.

@konn konn changed the title Implements the alignat environment for flexible math spacing Implements the alignedat environment for flexible math spacing Oct 17, 2017
@konn
Copy link
Collaborator Author

konn commented Oct 17, 2017

@kevinbarabash Thank you for everything!

Since it's an existing issue a separate PR is appropriate. Can you open a new issue for this?

OK, I've just separate this as Issue #941.

I'm going try to review this later this week.

Thanks!

@konn
Copy link
Collaborator Author

konn commented Oct 26, 2017

Just to caught up with master branch, I've just made a little rebasing on this PR.

@kevinbarabash
Copy link
Member

Sorry for not having reviewed the code for this yet. 😞 I've been rather busy lately. Going to try to get to it this weekend though.

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM

if (numMaths < curMaths) {
throw new ParseError(
"Too many math in a row: expected "
+ numMaths + ", but got " + curMaths,
Copy link
Member

Choose a reason for hiding this comment

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

style nit:

"Too many math in a row: " + 
`expected ${numMaths}, but got ${curMaths}`,

might be at little easier to read. Also, the indentation is a little funky here. Can you indent args that occur on new lines, e.g.

throw new ParseError(
    "Too many..." +
    `expected ${numMaths}...`,
    row);

@kevinbarabash kevinbarabash merged commit 1a640a4 into KaTeX:master Nov 12, 2017
@kevinbarabash
Copy link
Member

@konn thanks for the PR. Sorry again about the delay.

@konn konn deleted the alignat branch November 12, 2017 03:00
@konn
Copy link
Collaborator Author

konn commented Nov 12, 2017

@kevinbarabash No problem. Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants