feat: Support {CD} #2396
feat: Support {CD} #2396
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2396 +/- ##
==========================================
+ Coverage 93.83% 93.89% +0.06%
==========================================
Files 84 85 +1
Lines 6145 6277 +132
Branches 1260 1288 +28
==========================================
+ Hits 5766 5894 +128
- Misses 348 353 +5
+ Partials 31 30 -1
Continue to review full report at Codecov.
|
|
Okay, I've added type annotations and resolved my little merge problem. This PR is ready for review. |
|
@kevinbarabash Thank you for an excellent review. It really helped me to clarify what the code is doing. I've picked up comments in the code but I have not yet written the new tests. More to come. Also, there's a good description of |
| display="block" | ||
| > | ||
| <semantics> | ||
| <mtable rowspacing="0.24999999999999992em" |
There was a problem hiding this comment.
It's a computed number from https://github.com/KaTeX/KaTeX/blob/master/src/environments/array.js#L514-L519.
I'll add code that will round it to 4 places after the decimal.
There was a problem hiding this comment.
I think it's fine to leave this as is for now and handle it as part of #2460.
| scriptlevel="1" | ||
| > | ||
| <mpadded width="0" | ||
| lspace="-1width" |
There was a problem hiding this comment.
Is width supposed to be a number here?
There was a problem hiding this comment.
It's correct as is. The "-" prefix tells a lspace attribute that the value is an increment. And lspace accepts "1width" as a variable, which is a good thing because we don't know the numeric width.
So this <mpadded> creates a right-justified label.
| scriptlevel="1" | ||
| > | ||
| <mpadded width="0" | ||
| lspace="-1width" |
test/katex-spec.js
Outdated
| it("should fail if not is display mode", function() { | ||
| expect(`\\begin{CD}A @X<a<< B @>>b> C @>>> D\\\\@. @| @AcAA @VVdV \\\\@. E @= F @>>> G\\end{CD}`).not.toParse( | ||
| new Settings({displayMode: false}) | ||
| ); | ||
| }); | ||
| const displaySettings = new Settings({displayMode: true}); | ||
| it("should fail if the character after '@' is not in <>AV=|.", function() { | ||
| expect(`\\begin{CD}A @X<a<< B @>>b> C @>>> D\\\\@. @| @AcAA @VVdV \\\\@. E @= F @>>> G\\end{CD}`).not.toParse(displaySettings); | ||
| }); |
There was a problem hiding this comment.
It would be good to have a successfully test here as well. It looks like the display mode test also has an issue with the X coming after @. Having a successfully test case would help when determine which part of the other test cases are causing them not parse.
src/environments/cd.js
Outdated
| const firstLabel = {type: "ordgroup", mode: "math", body: []}; | ||
| const secondLabel = {type: "ordgroup", mode: "math", body: []}; |
There was a problem hiding this comment.
I've been using TypeScript for another project recently and it requires more of the types to be explicitly specified.
src/environments/cd.js
Outdated
| return parent; | ||
| }, | ||
| mathmlBuilder(group, options) { | ||
| return new mathMLTree.MathNode("mrow", |
There was a problem hiding this comment.
super style nit: extra space
|
This looks good to go. I'll merge it once the automated checks complete. |
|
Awesome work guys! Very cool to have commutative diagrams of some form in KaTeX! |
This (newly-merged feature) gets us nicer formatting for the commutative diagram in examples/solids and will likely come in handy elsewhere. KaTeX/KaTeX#2396
This (newly-merged feature) gets us nicer formatting for the commutative diagram in examples/solids and will likely come in handy elsewhere. KaTeX/KaTeX#2396
This (newly-merged feature) gets us nicer formatting for the commutative diagram in examples/solids and will likely come in handy elsewhere. KaTeX/KaTeX#2396
This PR implements the AMS
{CD}environment and therefore it is a partial response to #1834. Authors really want something more capable than{CD}, but this is doable now. Here's a screenshot showing the various arrows and labels.I have not yet done type annotations. If PR #2326 is going to merge soon, I may wait and try my hand at TypeScript instead of Flow.