Skip to content

KaTeX (4/n): Ignore classes that don't have CSS definition #1601

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

Merged
merged 2 commits into from
Jul 3, 2025

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented Jun 17, 2025

Stacked on top of #1609.

@rajveermalviya rajveermalviya changed the title content: Ignore classes that don't have CSS definition KaTeX (4/n): Ignore classes that don't have CSS definition Jun 17, 2025
@rajveermalviya rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Jun 17, 2025
@gnprice
Copy link
Member

gnprice commented Jun 18, 2025

Thanks!

The new commit here atop #1559:
cc657f1 content: Ignore classes that don't have CSS definition

looks like it's pretty simple and should be fairly easy to merge. After you split up #1452 as described at #1452 (comment), would you rebase this to be atop that first segment?

(It looks like it'd conflict with one of the commits there, so that seems like the simplest way to handle the conflict.)

If these PRs were new, I'd suggest instead squashing this into the similar commit in #1452. But given that previous revisions of #1452 have been out in releases now, I think it'll be good to avoid adding new functionality to #1452, and put it in new PRs instead, like this one — that'll help us keep track of what's changed from one release in a crisp way.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-4 branch 2 times, most recently from dfc036d to c34a6e1 Compare June 19, 2025 14:03
@rajveermalviya
Copy link
Member Author

Updated to add mathdefault class, as discussed here: #mobile-team > KaTeX survey results @ 💬.

@gnprice
Copy link
Member

gnprice commented Jun 24, 2025

I've included this PR in today's release 259: #announce > mobile releases @ 💬. (I cherry-picked its one commit into the release branch; the previous commits' changes were included in the previous release 258, as part of #1452.)

To copy into this thread what we found in chat (#mobile-team > KaTeX survey results @ 💬) using the survey script #1600: on a corpus from an open Zulip community about math,

  • Before this change, in release v30.0.258, about 45% of messages using TeX were fully supported.
  • After this change, so in release v30.0.259, about 80% of messages using TeX are fully supported.

@gnprice gnprice added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 2, 2025
@gnprice gnprice self-assigned this Jul 2, 2025
@gnprice gnprice force-pushed the pr-tex-content-4 branch from 2bb1c37 to cbeef34 Compare July 2, 2025 04:40
Comment on lines +354 to +364
case 'mbin':
case 'mpunct':
case 'nobreak':
case 'allowbreak':
case 'mathdefault':
Copy link
Member

Choose a reason for hiding this comment

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

This change looks good, but leaves it still a bit mysterious why these classes are there in the KaTeX output if we really don't need to do anything about them.

As it happens, while I was reviewing #1609 earlier today I came across a couple of threads in the KaTeX tracker that basically answer that question. So I've just pushed a commit on top which adds a comment linking to those:
cbeef34 content [nfc]: Explain why some KaTeX CSS classes are unused in its CSS

@gnprice
Copy link
Member

gnprice commented Jul 2, 2025

This is ready to merge as soon as #1609 is merged.

@gnprice gnprice force-pushed the pr-tex-content-4 branch from cbeef34 to 0498916 Compare July 3, 2025 06:08
@gnprice gnprice merged commit 0498916 into zulip:main Jul 3, 2025
1 check passed
@gnprice
Copy link
Member

gnprice commented Jul 3, 2025

And merged, after rebasing atop the merged #1609.

@gnprice gnprice mentioned this pull request Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants