-
-
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
Added support for bold italic symbols #1011
Conversation
Hey @aterenin, Thanks for the PR! Mind signing our Contributor License Agreement? When you've done so, go ahead and comment Yours truly, |
[clabot:check] |
CLA signature looks good 👍 |
@aterenin thanks for the PR. I thought for some reason we didn't have the fonts or the metrics but apparently we have both. This could probably use a screenshot test. Please see https://github.com/Khan/KaTeX/blob/master/CONTRIBUTING.md#screenshot-tests for details. |
Tests implemented. I'm currently in the middle of travel (coded up this PR while on a flight) so I'm not equipped to install docker to run the screenshotter at the moment. It'd be great if someone could run it and add to the PR, otherwise I'll handle it later. |
test/katex-spec.js
Outdated
expect(markup).toContain("<mi mathvariant=\"bold-italic\">\u03A9</mi>"); // \Omega | ||
expect(markup).toContain("<mi>\u0131</mi>"); // \imath | ||
expect(markup).toContain("<mo>+</mo>"); | ||
}); |
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.
We've started using snapshot tests for testing MathML markup, see mathml-spec.js. Can you add a test there instead?
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 do not understand what those tests do or how they work. There are no similar tests implemented for \mathbf or any similar command. Could you either provide reference on how to implement that or accept as-is and move to mathml-spec.js later, when \mathbf is moved there too?
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.
it("should render \\boldsymbol{" + contents + "} with the correct mathvariants", () => {
const tex = "\\boldsymbol{" + contents + "}";
const tree = getParsed(tex);
const markup = buildMathML(tree, tex, defaultOptions).toMarkup();
expect(markup).toMatchSnapshot();
});
You'll need to run the tests once locally in order to update the katex-spec.js.snap
. Include the changes to that file in this diff and you should be good.
The tests work by comparing the output of future runs agains the snapshot. The snapshot can also be regenerated. I should add more detail to https://github.com/Khan/KaTeX/blob/master/CONTRIBUTING.md#jest-tests along with an npm
script to regenerate the snapshots when things change.
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've added a task for myself: #1013.
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.
Done
test/screenshotter/ss_data.yaml
Outdated
@@ -57,6 +57,7 @@ BinCancellation: | | |||
\end{array} | |||
BinomTest: \dbinom{a}{b}\tbinom{a}{b}^{\binom{a}{b}+17} | |||
BoldSpacing: \mathbf{A}^2+\mathbf{B}_3*\mathscr{C}' | |||
BoldSymbol: \boldsymbol{\omega}+\boldsymbol{\Omega}+\boldsymbol{A}+\boldsymbol{x} |
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.
You'll need to generate screenshots and include those files as well. Let me know if you have issues running make screenshots
.
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.
make screenshots
does not work. I get the following:
/Users/aterenin/Git/KaTeX/node_modules/selenium-webdriver/lib/promise.js:654
throw error;
^
Error: Timed out waiting for the WebDriver server at http://127.0.0.1:49315/hub
at onError (/Users/aterenin/Git/KaTeX/node_modules/selenium-webdriver/http/util.js
:87:11)
...
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.
@aterenin did docker
start correctly?
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.
Figured out the problem: I previously didn't install the correct docker
.
Screenshots added and pushed.
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.
@aterenin cool. I take it npm install docker
installs a client. I should update the README.md for the Screenshot to point to link docker's download page or homepage so there's less confusion.
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.
It might be good to include some other symbols in this such as \sum
and \int
.
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.
Done
Great stuff! Would it be possible to add |
@jeanm |
Not in LaTeX, true, but do these differences still apply here? And even in LaTeX, the differences are so minimal that they don't matter in the majority of real-world use cases I see in papers in my field. It would be annoying to have to rewrite a bunch of formulae that use |
@jeanm we'd like avoid implementing commands where the semantics don't match LaTeX's as much as possible. See https://github.com/khan/KaTeX#rendering-options for how to define your own macros. |
@aterenin Awesome feature addition! Thanks for working on this. In #585, I concluded that we should aim to support |
I guess I'm okay with the alias too as long as we communicate the short coming and at the very least have an issue in github to track the issue with which command is non-compliant. |
There's something to be said for simplicity, especially given people can define a macro for I can easily add an alias for |
It's sounds like the general consensus to add the alias. |
Alias added. |
<mo> | ||
+ | ||
</mo> | ||
</mrow> |
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 think everything in this row should have mathvariant="bold-italic"
set.
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.
Are you sure? These include the imaginary number symbol, as well as the plus symbol, for which mathvariant="bold"
is not set even for \mathbf
.
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.
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.
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.
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.
Turns out the same situation occurs for \mathit
- using that as a template, I was able to implement \imath
and 2
. +
is still not correct, because for some reason the font doesn't get applied to it, will look at that shortly.
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.
Done. Please see new screenshots that have been pushed.
Thanks for adding the snapshot test. Almost there, just need to add |
Test updated. |
src/buildCommon.js
Outdated
"\\imath", // dotless i | ||
"\\jmath", // dotless j | ||
"+", // plus symbol | ||
"-", // minus symbol |
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 looked at the font file for KaTeX_MainBold and realized that there are more symbols in it that we should show in bold, e.g. =
, <
, >
, etc. We could use lookupSymbol
to determine whether we have a bold variant or not, that way we can include all of them without the extra table.
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.
Agreed; this should ideally not be hard coded.
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've removed the hardcoded symbols - this was done for mathit
so I copied the approach - and am now using lookupSymbol.
src/buildCommon.js
Outdated
): {| fontName: string, fontClass: string |} { | ||
if (/[0-9]/.test(value.charAt(0)) || | ||
// glyphs for \imath and \jmath do not exist in Math-BoldItalic so | ||
// we need to use Main-BoldItalic instead |
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.
nit: Main-BoldItalic -> Main-Bold
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.
Fixed
// "mathit" and "boldsymbol" are missing because they require the use of two | ||
// fonts: Main-Italic and Math-Italic for "mathit", and Math-BoldItalic and | ||
// Main-Bold for "boldsymbol". This is handled by a special case in makeOrd | ||
// which ends up calling mathit and boldsymbol. |
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.
nice comment
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.
LGTM. Thanks for all the revisions.
@aterenin thanks for fixing my lint. 😅 |
No worries. I pushed a 1-character fix to make CI happy. Any idea when this'll make it into the release? With this added, I can now switch the live version of my blog from MathJax to KaTeX. Only thing missing for me now is auto equation numbering, which I've no doubt will make it in eventually. And if I'm allowed to wish for things MathJax can't do |
@aterenin thanks for this PR. |
I'm using KaTeX from the CDN, which is using version 0.9.0-alpha2. Am I correct that support for bolded italics has not yet been included in the CDN? Thank you! |
@stapeleliz that's right. I need to publish a new release. |
This adds support for
\boldsymbol
, as requested in #633 and #585.It uses the bold italic fonts already present in KaTeX. It works with both Greek and Latin letters.