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.

Sign up for GitHub

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

Jump to bottom

Add \iff, \implies, \impliedby support #697

Merged
merged 1 commit into from May 17, 2017
Merged

Add \iff, \implies, \impliedby support #697

merged 1 commit into from May 17, 2017

Conversation

edemaine
Copy link
Member

Fix #190 via @ronkok's code.

I thought about tests but couldn't think of anything that interesting. A expect(...).toParseLike doesn't seem to check anything. I could add a screenshot test, but it is also pretty "boring".

@edemaine edemaine mentioned this pull request May 11, 2017
Closed
@ronkok
Copy link
Collaborator

ronkok commented May 11, 2017

That looks good. And kudos to @gagern, who made macros possible.

kevinbarabash
kevinbarabash requested changes May 13, 2017
View reviewed changes
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.

Can you add tests to katex-spec.js that very that these macros "build" using the toBuild matcher?

src/macros.js Outdated

// \def\iff{\DOTSB\;\Longleftrightarrow\;}
// \def\implies{\DOTSB\;\Longrightarrow\;}
// \def\impliedby{\DOTSB\;\Longleftarrow\;}
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 add a TODO above these to indicate that this is what we want the the implementation of these commands to eventually look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

@edemaine
Add \iff, \implies, \impliedby support. Fix KaTeX#190.
46f3418 
* Based on @ronkok's code, but without \mathrel as it's absent
  from amstex.sty.
* Add toBuild tests to ensure these operators expand.
@edemaine
Copy link
Member Author

@kevinbarabash Implemented both your suggestions (and rebased). Thanks for the feedback!

I also noticed that, while @ronkok's code had a \mathrel surrounding the operators, the amstex.sty code did not, so I switched to the latter.

@ronkok
Copy link
Collaborator

ronkok commented May 16, 2017

I'm good with the change. The span still is coded as a class="mrel".

@kevinbarabash kevinbarabash merged commit 78be8d4 into KaTeX:master May 17, 2017
@kevinbarabash
Copy link
Member

kevinbarabash commented May 17, 2017 •
edited

@edemaine thanks for adding these new commands and @ronkok for the providing the code in the issue report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinbarabash kevinbarabash kevinbarabash approved these changes

Assignees
No one assigned
Labels
GH review: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@edemaine @ronkok @kevinbarabash