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

Added support for bold italic symbols #1011

Merged
merged 13 commits into from
Dec 13, 2017
Merged

Added support for bold italic symbols #1011

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0c70cb0
added support for \boldsymbol
aterenin Dec 6, 2017
10054f3
added \boldsymbol tests
aterenin Dec 6, 2017
6103e5e
added \boldsymbol screenshots
aterenin Dec 8, 2017
8ae294a
Merge branch 'master' into master
aterenin Dec 8, 2017
16960fa
added \bm, snapshot, updated screenshots
aterenin Dec 8, 2017
36ef8f2
updated snapshot for \boldsymbol
aterenin Dec 9, 2017
165809d
properly display \imath and 2 in boldsymbol
aterenin Dec 9, 2017
4fe075e
implemented \boldsymbol for +
aterenin Dec 9, 2017
6e2aec5
Merge branch 'master' into master
Dec 12, 2017
136b689
fix typo, remove boldsymbolLetters hardcode
aterenin Dec 13, 2017
f741c2f
Merge branch 'master' of https://github.com/aterenin/KaTeX
aterenin Dec 13, 2017
fdd231e
make comment about using Main-Bold more general
kevinbarabash Dec 13, 2017
a7b1d13
fixed trailing space CI error
aterenin Dec 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions test/katex-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,6 +1713,19 @@ describe("A MathML font tree-builder", function() {
expect(markup).toContain("<mo>+</mo>");
});

it("should render \\boldsymbol{" + contents + "} with the correct mathvariants", function() {
const tex = "\\boldsymbol{" + contents + "}";
const tree = getParsed(tex);
const markup = buildMathML(tree, tex, defaultOptions).toMarkup();
expect(markup).toContain("<mi mathvariant=\"bold-italic\">A</mi>");
expect(markup).toContain("<mi mathvariant=\"bold-italic\">x</mi>");
expect(markup).toContain("<mn>2</mn>");
expect(markup).toContain("<mi mathvariant=\"bold-italic\">\u03c9</mi>"); // \omega
expect(markup).toContain("<mi mathvariant=\"bold-italic\">\u03A9</mi>"); // \Omega
expect(markup).toContain("<mi>\u0131</mi>"); // \imath
expect(markup).toContain("<mo>+</mo>");
});
Copy link
Member

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?

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


it("should render \\mathbf{" + contents + "} with the correct mathvariants", function() {
const tex = "\\mathbf{" + contents + "}";
const tree = getParsed(tex);
Expand Down
1 change: 1 addition & 0 deletions test/screenshotter/ss_data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Member

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.

Copy link
Collaborator Author

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)
...

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Boxed: \boxed{F=ma} \quad \boxed{ac}\color{magenta}{\boxed{F}}\boxed{F=mg}
Cases: |
f(a,b)=\begin{cases}
Expand Down