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.

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

Improve \sqrt #810

Merged
merged 11 commits into from
Aug 23, 2017
Merged

Improve \sqrt #810

merged 11 commits into from
Aug 23, 2017

Conversation

ronkok
Copy link
Collaborator

@ronkok ronkok commented Aug 20, 2017

Make \sqrt out of inline SVGs to ensure a perfect match at the junction between surd and viniculum.

ronkok and others added 5 commits August 20, 2017 09:55
Make \sqrt out of inline SVGs to ensure a perfect match at the junction between surd and viniculum.
regenerate screenshot tests with sqrts
@kevinbarabash
Copy link
Member

sqrt

Green is LaTeX, red is KaTeX. The spacing between the horizontal rules in the superscripts is improved over the last iteration of the inline SVG.

@kevinbarabash
Copy link
Member

kevinbarabash commented Aug 22, 2017

Some of the radical symbols seem to be missing padding on the left that's built into the glyph.

before:
screen shot 2017-08-22 at 12 08 54 am

after:
screen shot 2017-08-22 at 12 09 05 am

Highlighting the outermost sqrt of the first superscript shows that there is no left padding in that sqrt.
screen shot 2017-08-22 at 12 06 18 am

The large sqrt symbols seem to be the ones that are affected as there should be a gap between the first superscript and the second. There should also be more space to the left of the leftmost sqrt.

I looked at KaTeX_Size1-Regular and KaTeX_Size2-Regular fonts in opentype.js to confirm that there is indeed left padding built into the glyphs.

screen shot 2017-08-22 at 12 06 49 am

@ronkok can you update the inline SVGs to include the missing left padding? Sorry for not catching this in in the original review.

@ronkok
Copy link
Collaborator Author

ronkok commented Aug 22, 2017

I was hoping to avoid this. I didn't catch it before I minified the SVG code. Before that point, I have things somewhat automated, but afterwards a change is pretty tedious. No matter. I'll do it.

I have already added the correct leading advance to the main surd. I'll go and do the other five.

@kevinbarabash
Copy link
Member

Before that point, I have things somewhat automated, but afterwards a change is pretty tedious. No matter. I'll do it.

Do you have a script? Would it be possible to add the padding via bounding box change?

@ronkok
Copy link
Collaborator Author

ronkok commented Aug 22, 2017

I have scripts that do matrix transformations of the original geometry. I shift all of them so that the SVG has no negative coordinates, because IE 9 reportedly doesn't deal well with negative coordinates.

But then there are non-scripted changes to make the viniculum absurdly long. And the tall surd got some custom changes of its own.

Don't worry about it. I have found copies from an intermediate step in the process. That will make things pretty easy for sizes 1 thru 4.

Edit the SVG paths so that they have the correct left bearing and advance width values.

This will correct the spacing on the left side of each surd and it will also improve the placment of a root indice.
@ronkok
Copy link
Collaborator Author

ronkok commented Aug 22, 2017

@kevinbarabash It turned out to be pretty easy. Thank you for the nudge.

ronkok and others added 2 commits August 22, 2017 16:14
In the `main` size, delimiters *do* scale with scriptstyle and scriptscriptstyle.
@kevinbarabash
Copy link
Member

sqrt

Left-padding on surds looks good now.

@kevinbarabash
Copy link
Member

screen shot 2017-08-22 at 9 13 24 pm

MathJax output for comparison.

update screenshot images containing sqrts
Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM

src/delimiter.js Outdated
sizeMultiplier = newOptions.sizeMultiplier;

span.height = 1 / sizeMultiplier;
span.height = 1 * sizeMultiplier;
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad you were able to simplify this.

@kevinbarabash kevinbarabash merged commit e88256b into KaTeX:master Aug 23, 2017
@ronkok
Copy link
Collaborator Author

ronkok commented Aug 23, 2017

Yes, it took a few tries to focus in on the target, but we got there eventually. Thanks for your help and your patience.

@kevinbarabash
Copy link
Member

@ronkok thank you so much for this PR. This fixes #60 which has been around for almost three years. \sqrt look awesome!

@edemaine
Copy link
Member

Nice work guys!

@jpzwarte
Copy link

Very cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants