-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adds math commands, HTML rendering, and screenshotter tests. #291
Conversation
The commit message writes “Part 2 will add…” which should read “Part 3…”. The screenshot tests don't cover the question of the scope of the new commands, discussed in #132 (comment). |
I decided to leave out the commands defined using |
I think the scoping checks, or at least one scoping check, makes sense for |
I meant to do that and forgot. Thanks for reminding me. |
var font = options.font; | ||
if (font) { | ||
if (font === "mathit" || utils.contains(["\u0131", "\u0237"], value)) { | ||
return mathit(value, mode, color, classes.concat(["mathit"])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this add the "mathit" class twice sometimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
The |
@@ -308,13 +361,72 @@ var spacingFunctions = { | |||
} | |||
}; | |||
|
|||
var greekCapitals = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this above all of the functions that use it?
4f0c353
to
c67fcd2
Compare
I'll have to wait until part 1 gets merged before I can tackle "test that fonts don't leak past the end of the group when nesting". |
"\\Bbb": "\\mathbb", | ||
"\\bold": "\\mathbf", | ||
"\\frak": "\\mathfrak" | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these font aliases work in LaTeX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested these and they do.
This looks good! I was a little confused about the spacing between the |
@xymostech let me know when you've sneaked it in and I'll rebase and redo the screenshots. |
4dd3b2a
to
67538d2
Compare
|
||
return makeSpan( | ||
["katex-logo"], [k, a, t, e, x], options.getColor()); | ||
["katex-logo", "mord"], [k, a, t, e, x], options.getColor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snuck in the "mord"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
A couple of the screenshots are differently, although they don't look different when using github's onion skinning thingy. In particular these are LimitControls-firefox.png and UnsupportedCmds-firefox.png. They're probably suffering from the subpixel differences fixed in #324. |
@xymostech this is ready for review now. |
This all looks good! Sorry it took me so long to review this! |
@xymostech no worries. Thank you. I'll rebase later today. |
I'm so happy that this is going forward now! |
This is part 2 of 3. Part 1 added new fonts metrics. Part 2 will add MathML support and unit tests.
67538d2
to
fd2d58f
Compare
Adds math commands, HTML rendering, and screenshotter tests.
This is part 2 of 3. Part 1 added new fonts metrics. Part 3 will add MathML support and unit tests.
Depends on #290.
TODO:
\frak
,\bold
, and\Bbb