katex: Explain reason for ignoring .vlist-t2 and .vlist-s CSS classes
#1832
Conversation
rajveermalviya
added
a-tex
lib/model/katex.dart
Outdated
| // We ignore the styling for .vlist-t2 CSS class on the root vlist | ||
| // span, and also the styling for .vlist-s CSS class on the empty span | ||
| // in the first .vlist-r span, because in KaTeX they only exists as a |
There was a problem hiding this comment.
The vlist-t2 and vlist-s classes should always appear in tandem, right? And I think that's a critical part of the reasoning here — each of them does have an effect, of 2px, but those effects cancel each other out.
So as I said on the issue (#1716 (comment)), we should verify that the vlist-t2 class appears exactly when a .vlist-s span is present.
There was a problem hiding this comment.
Updated the comment with a link to KaTeX implementation showing both classes are always present in tandem, and also mentioning that I verified that tehy cancel each others effect out.
rajveermalviya
force-pushed
the
pr-katex-vlist-t2
branch
from
cb3f838 to
fe2818f
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
gnprice
force-pushed
the
pr-katex-vlist-t2
branch
from
fe2818f to
32d2096
Compare
|
Cool, that's helpful, thanks. What I really had in mind by "verify" was to have the parser verify that the HTML it sees matches that description, like we do already for facts like "there's at most two rows; any second row has one cell; that one cell has no inline styles other than I feel like the reasoning, and its relationship to the logic in this parser method, can still be clearer, though. So I've just pushed a couple of added commits to try to do that: PTAL and fact-check that the details in those comments all look right to you. (I also made a commit-message tweak: the first/existing commit is NFC, since it only touches comments.) |
|
Thanks for doing this, those comments LGTM! |
This reflects the condition that's directly encoded by this boolean. (Through several conditionals in the rest of the function, we end up confirming that this value is true just if the element does in fact have two `.vlist-r` children, as the old name suggests. But that comes later and more indirectly.)
…t it Mostly this moves and revises comments. Also rename one local to "depthStrut" to match its name in the KaTeX JS, which we also now use in the explanatory comment.
|
Thanks! Merging. |
gnprice
force-pushed
the
pr-katex-vlist-t2
branch
from
32d2096 to
ce8802b
Compare
Fixes: #1716