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

Switch makeGlue from .mord .rule to .mspace #1295

Merged
merged 2 commits into from May 8, 2018

Conversation

edemaine
Copy link
Member

@edemaine edemaine commented May 8, 2018

Fix #1289

I haven't run screenshot tests yet; I'm curious to see whether this messes with anything.

@codecov
Copy link

codecov bot commented May 8, 2018

Codecov Report

Merging #1295 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1295   +/-   ##
=======================================
  Coverage   83.88%   83.88%           
=======================================
  Files          61       61           
  Lines        3978     3978           
  Branches      661      661           
=======================================
  Hits         3337     3337           
  Misses        543      543           
  Partials       98       98
Impacted Files Coverage Δ
src/buildCommon.js 90.41% <100%> (ø) ⬆️

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 ba24d20...3e4b5aa. Read the comment docs.

@edemaine
Copy link
Member Author

edemaine commented May 8, 2018

Amazingly, only two screenshots were affected. Here's the diff:

StrikeThrough:
strikethrough-chrome-diff

StrikeThroughColor:
strikethroughcolor-chrome-diff

The result of this PR is in green/yellow, with more spaces. Somewhat easier to see in the Github diff. I think this shows that the change fixes some spacing bugs, properly treating the + as a binary operator. One example of the LaTeX source code is \bcancel 5 +\bcancel{5ay}.

@ronkok
Copy link
Collaborator

ronkok commented May 8, 2018

Yay! That solves a mystery. I couldn't figure out why \cancel wasn't working when adjacent to "+". So this addresses, at least in part, issue #1080.

@kevinbarabash
Copy link
Member

@edemaine could you post diff images between the new render and the LaTeX rendering?

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.

I'm glad that this didn't change much and ended up improving things a bit in the places it did change.

@kevinbarabash kevinbarabash merged commit 5c159ab into KaTeX:master May 8, 2018
@edemaine
Copy link
Member Author

edemaine commented May 8, 2018

I posted it on #1080 where you can compare to the old diff. I'm afraid it's not much better... but hopefully a step in the right direction.

strikethrough

edemaine added a commit to edemaine/KaTeX that referenced this pull request May 8, 2018
kevinbarabash pushed a commit that referenced this pull request May 10, 2018
* Line breaks for inline formulas

* Basic support for \allowbreak and \nobreak

* Fix spacing around \nobreak, and add documentation

* Backwards-compatibility _getBuilt to fix tests

* Put operator spacing on same line as operator

* One approach to ~

* Simplify \allowbreak/\nobreak, make ~/\nobreakspace prevent line breaks

* Adapt to #1295

* Prevent wrapping within a .base

* Implement \hspace* properly

* Fix flow error

* Update comment for regularSpace

* Update screenshots

* Move `width: min-content` from .katex into .base

* Fix screenshot

* Add min-width rule to .vlist-s

* Factor out hasClass method

* Cleanup nobreak test

* Pull out buildHTMLUnbreakable

* Fix \hspace* test (no longer the same as \hspace)

* Fix \nobreak handling

* Add screenshot test
@ylemkimon ylemkimon mentioned this pull request Aug 18, 2018
1 task
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.

None yet

3 participants