Remove 'katex' from @khanacademy/perseus #1428
Conversation
| @@ -1,7 +1,6 @@ | |||
| import * as React from "react"; | |||
|
|
|||
| import Graph from "../graph"; | |||
| // TODO(scottgrant): Katex is unavailable here. Fix! | |||
There was a problem hiding this comment.
I couldn't even make sense of what was to be done here so I'm deleting this comment.
| // support (yet) with \begin{aligned}...\end{aligned} which renders | ||
| // the same is supported by KaTeX. It does the same for align*. | ||
| // TODO(kevinb) update content to use aligned instead of align. | ||
| const tex = node.content.replace(/\{align[*]?\}/g, "{aligned}"); |
There was a problem hiding this comment.
MathJax supports the \begin{align} macro just fine (tested in Storybook).
There was a problem hiding this comment.
Removing this code might break the exercise editor, which still uses KaTeX behind the scenes to validate that the input TeX can render with KaTeX. This gets complicated to reason about, because the reason the editor does that is to ensure that TeX written by content creators is compatible with old versions of the mobile app, which use KaTeX (and old versions of Perseus which have this replacement logic, so \begin{align} will work fine there).
I think we could move this logic to the exercise editor and it would be fine. But I'd lean toward keeping this code the same until we can remove KaTeX everywhere. If you agree, can you restore this code and add a TODO comment here referencing LEMS-1608?
|
Size Change: -292 B (-0.03%) Total Size: 854 kB
ℹ️ View Unchanged
|
| @@ -318,7 +318,6 @@ export type APIOptions = Readonly<{ | |||
|
|
|||
| type TeXProps = { | |||
| children: string; | |||
| katexOptions?: any; | |||
There was a problem hiding this comment.
I checked and this prop/string is not used in webapp anywhere!
There was a problem hiding this comment.
Git(hub) doesn't seem to recognize that this is a rename.
jeremywiebe
requested review from
benchristel,
Myranae,
nicolecomputer and
a team
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
| * If the type of contents is string, the contents will be rendered with | ||
| * KaTeX. Otherwise, the content will be assumed to be a DOM node and will | ||
| * be appended inside the tooltip. | ||
| * If the type of contents is string, the contents will be rendered as TeX |
There was a problem hiding this comment.
Nit: TeX is the input language, so I think it would be more accurate to say that "the contents will be interpreted as TeX and rendered as DOM elements"
| // support (yet) with \begin{aligned}...\end{aligned} which renders | ||
| // the same is supported by KaTeX. It does the same for align*. | ||
| // TODO(kevinb) update content to use aligned instead of align. | ||
| const tex = node.content.replace(/\{align[*]?\}/g, "{aligned}"); |
There was a problem hiding this comment.
Removing this code might break the exercise editor, which still uses KaTeX behind the scenes to validate that the input TeX can render with KaTeX. This gets complicated to reason about, because the reason the editor does that is to ensure that TeX written by content creators is compatible with old versions of the mobile app, which use KaTeX (and old versions of Perseus which have this replacement logic, so \begin{align} will work fine there).
I think we could move this logic to the exercise editor and it would be fine. But I'd lean toward keeping this code the same until we can remove KaTeX everywhere. If you agree, can you restore this code and add a TODO comment here referencing LEMS-1608?
| @@ -15,8 +15,8 @@ function findChildOrAdd(elem: any, className: string) { | |||
| } | |||
|
|
|||
| export default { | |||
| // Process a node and add math inside of it. This attempts to use KaTeX to | |||
| // format the math, and if that fails it falls back to MathJax. | |||
| // Process a node and add math inside of it. This uses MathJax | |||
There was a problem hiding this comment.
| // Process a node and add math inside of it. This uses MathJax | |
| // Process a node and add math inside of it. This uses MathJax to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1428 +/- ##
==========================================
+ Coverage 69.62% 71.30% +1.67%
==========================================
Files 504 508 +4
Lines 105597 105669 +72
Branches 7650 11489 +3839
==========================================
+ Hits 73521 75345 +1824
+ Misses 31960 30324 -1636
+ Partials 116 0 -116
... and 125 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Summary:
KaTeX was retired from use in Perseus in 2023. Today, I noticed that we still reference the
katexpackage in the perseuspackage.json, but we never import it anywhere in the code.So I'm removing that dependency. I've also updated some code comments and storybook references to not mention KaTeX anymore. I've left some references to CSS classes and the KaTeX_Main font ref. I'm unsure if they might be important somewhere (especially if host applications may still set it outside of Perseus).
So this doesn't fully remove all references to katex, but it moves us another step closer.
Issue: "none"
Test plan:
yarn tscyarn lintI ran storybook and opened the
ZoomableTexstories and played with them.I also went to the
EditorPagestory and pasted in the following Tex into the Question field (which uses the macro that we used to have to replace for KaTeX, but MathJax supports properly: switching\begin{align}to\begin{aligned}).