Improve JS spacing by ylemkimon · Pull Request #1103 · KaTeX/KaTeX

ylemkimon · 2018-01-27T19:23:56Z

Remove dummy spans for spacing around \href: Spacing around \href(anchor) is handled in the html.buildExpression, and currently spacings around \href are duplicated
Make bin cancellation aware of children of fragment and anchor
~~Fix tight spacing not applied: Fix spacing code for "tight" styles #1102~~ Fixed by use correct spacing with tight styles #1106
Add surrounding argument to html.buildExpression: it is an array consisting type of nodes that will be added to the left and the right, and if given, will be used to determine bin cancellation and spacings of outermost nodes. See delimsizing.js for its usage.

TODO:

spacings inside \href: Spacing inside \href is wrong #1108
add/update tests and screenshots

Spacing around \href is handled in the `buildHTML`

+ Added getOutermostNode function

It is an array consisting type of nodes that will be added to the left and the right, and if given, will be used to determine bin cancellation and spacings of outermost nodes.

k4b7 · 2018-01-28T17:13:09Z

src/functions/delimsizing.js

        // Build the inner expression
-        const inner = html.buildExpression(group.value.body, options, true);
+        const inner = html.buildExpression(group.value.body, options, true,
+            [null, "mclose"]);


This seems to be the on place where the surrounding param is used. Do you for other situations where we'll need the first value instead of just the second?

@kevinbarabash Not yet, but I think it makes sense to have for both sides.

k4b7 · 2018-01-28T17:21:41Z

src/buildHTML.js

+    nonSpaces.unshift(surrounding[0] &&
+        new makeSpan([surrounding[0]], [], options));
+    nonSpaces.push(surrounding[1] &&
+        new makeSpan([surrounding[1]], [], options));


Is it necessary to create the spans if surrounding[0] or surrounding[1] is null or undefined?

@kevinbarabash It's for easy manipulation of nonSpaces. This way, we can safely assume that nonSpaces[0] and nonSpaces[nonSpaces.length -1] are dummy spans.

I would rewrite this as:

const nonSpaces = [ surrounding[0] && makeSpan([surrounding[0]], [], options)), rawGroups.filter(group => group && group.classes[0] !== "mspace"), surrounding[1] && makeSpan([surrounding[1]], [], options)), ];

This avoids the unshift which could be costly depending on the length of the array and better shows that the surrounding array is being used to surround something.

Note: new isn't required for makeSpan.

I can't wait until we have everything converted to flow so that we can better enforce some of these assumptions.

k4b7 · 2018-01-28T17:22:50Z

src/functions/delimsizing.js

-            const dimension =
-                calculateSize(activeSpacings[lastChildType]["mclose"], options);
-            glue.style.marginRight = `${dimension}em`;
-            inner.push(glue);


Now this isn't included in inner/children. Won't that affect the results of getOutermostNode?

@kevinbarabash The html.buildExpression inserts the appropriate spacing between the last child and mclose.

That's pretty cool. Glad you were able to figure out a way to get all the spacing code in one place.

k4b7 · 2018-01-28T17:24:12Z

src/functions/href.js

-            }
-        }
-        return new buildCommon.makeAnchor(href, classes, elements, options);
+        return new buildCommon.makeAnchor(href, [], elements, options);


k4b7 · 2018-01-28T17:24:55Z

src/buildHTML.js

+        const right = getOutermostNode(nonSpaces[i], "right");
+        if (right.classes[0] === "mbin" &&
+                isBinRightCanceller(nonSpaces[i + 1], isRealGroup)) {
+            right.classes[0] = "mord";


k4b7 · 2018-02-01T01:19:32Z

@ylemkimon are you still planning to do the TODOs in the top comment? Did you want me to comment on the code as is before adding tests/updating screenshots?

k4b7 · 2018-02-01T01:59:23Z

whoops... wrong PR

ylemkimon · 2018-02-01T05:47:00Z

@kevinbarabash Yes, and if you don’t mind, yes, please. Though, I’m not sure what is best way to add spacings recursively for \href and \class in #1074.

k4b7 · 2018-02-11T20:44:12Z

src/buildHTML.js

 */
-export const buildExpression = function(expression, options, isRealGroup) {
+export const buildExpression = function(expression, options, isRealGroup,
+        surrounding = []) {


Maybe make this surrounding = [null, null]. I think eventually we'll want to enforce that this is a 2-tuple with flow.

k4b7

LGTM. I think the TODOs in the summary could be handled in a separate PR.

codecov · 2018-02-11T20:50:51Z

Codecov Report

Merging #1103 into master will increase coverage by 0.35%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #1103      +/-   ##
==========================================
+ Coverage   79.01%   79.37%   +0.35%     
==========================================
  Files          59       59              
  Lines        3875     3865      -10     
  Branches      655      648       -7     
==========================================
+ Hits         3062     3068       +6     
+ Misses        672      662      -10     
+ Partials      141      135       -6

Impacted Files	Coverage Δ
src/functions/href.js	`72.72% <0%> (+34.63%)`	⬆️
src/functions/delimsizing.js	`65.85% <100%> (+1.8%)`	⬆️
src/buildHTML.js	`93.53% <92.3%> (+1.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b2101f...44381dc. Read the comment docs.

k4b7

A couple of minor nits before merging:

avoid push/unshift on nonSpaces
don't use new on makeSpan

k4b7 · 2018-02-13T12:01:36Z

I tried checking the checkboxes but apparently you can't do that to checkboxes inside review summaries.

k4b7 · 2018-02-13T12:10:07Z

@ylemkimon thanks for the PR.

ylemkimon added 4 commits January 28, 2018 03:50

Remove dummy spans for spacing around \href

b1aa6c0

Spacing around \href is handled in the `buildHTML`

Make bin cancellation aware of children of fragment and anchor

b65f1af

+ Added getOutermostNode function

Fix tight spacing not applied

1ad404f

Add surrounding argument to html.buildExpression

90b0cfd

It is an array consisting type of nodes that will be added to the left and the right, and if given, will be used to determine bin cancellation and spacings of outermost nodes.

ylemkimon requested a review from k4b7 January 27, 2018 19:25

ylemkimon and others added 3 commits January 28, 2018 04:29

Fix html.buildExpression call in leftright

4331a04

Merge branch 'master' into js_spacing

8822d11

Add dummy span only when given

f7d3781

k4b7 reviewed Jan 28, 2018

View reviewed changes

k4b7 added GH review: accepted and removed GH review: accepted labels Feb 1, 2018

ylemkimon mentioned this pull request Feb 1, 2018

[PoC]Plugin architecture using webpack (HTML \class extension) #1074

Closed

4 tasks

k4b7 reviewed Feb 11, 2018

View reviewed changes

k4b7 approved these changes Feb 11, 2018

View reviewed changes

Merge branch 'master' into js_spacing

504f138

Merge branch 'master' into js_spacing

36df514

k4b7 requested changes Feb 11, 2018

View reviewed changes

ylemkimon added 2 commits February 13, 2018 14:33

Update buildHTML.js

b58c3dd

Merge branch 'master' into js_spacing

44381dc

k4b7 approved these changes Feb 13, 2018

View reviewed changes

k4b7 merged commit 439cea3 into KaTeX:master Feb 13, 2018

ylemkimon deleted the js_spacing branch May 20, 2018 11:59

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

ylemkimon deleted the js_spacing branch May 20, 2018 11:59

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Uh oh!

Conversation

ylemkimon commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Feb 1, 2018

Uh oh!

k4b7 commented Feb 1, 2018

Uh oh!

ylemkimon commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

k4b7 left a comment

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Feb 13, 2018

Uh oh!

k4b7 commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ylemkimon commented Jan 27, 2018 •

edited

Loading

codecov bot commented Feb 11, 2018 •

edited

Loading