-
Notifications
You must be signed in to change notification settings - Fork 350
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MathJax supports the \begin{align}
macro just fine (tested in Storybook).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I looked up our new mobile deprecation policy and I believe we will be able to completely remove KaTeX starting September 18th!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to move forward and land this, so I'll do as Ben suggests and restore this code with a new TODO. Thanks for thinking of that Ben!
Size Change: -4.55 kB (-0.53%) Total Size: 849 kB
ℹ️ View Unchanged
|
@@ -318,7 +318,6 @@ export type APIOptions = Readonly<{ | |||
|
|||
type TeXProps = { | |||
children: string; | |||
katexOptions?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and this prop/string is not used in webapp anywhere!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Git(hub) doesn't seem to recognize that this is a rename.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing the \begin{align}
replacement will break the exercise editor. I left a note inline. So we either need to keep the replacement code, or move it to the exercise editor. Otherwise, this looks good!
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1428 +/- ##
==========================================
+ Coverage 69.62% 70.21% +0.59%
==========================================
Files 504 509 +5
Lines 105597 104928 -669
Branches 7650 10716 +3066
==========================================
+ Hits 73521 73677 +156
+ Misses 31960 31251 -709
+ Partials 116 0 -116
... and 171 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (0e398f4) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1428 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1428 |
Co-authored-by: Ben Christel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Major Changes - [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead ### Minor Changes - [#1455](#1455) [`8b0268215`](8b02682) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon graphs to have weighted lines only on keyboard focus - [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs - [#1428](#1428) [`eb9f3f9c0`](eb9f3f9) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Drop katex dependency - no longer used - [#1472](#1472) [`4c2ace57a`](4c2ace5) Thanks [@benchristel](https://github.com/benchristel)! - Add keyboard controls to Mafs angle graphs - [#1487](#1487) [`53031f8df`](53031f8) Thanks [@Myranae](https://github.com/Myranae)! - Make graphs non-interactive via keyboard when their question has been answered correctly - [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs - [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) ### Patch Changes - [#1474](#1474) [`a8aaac339`](a8aaac3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor type fixes in the `ImageLoader` and `SvgImage` components - [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor - [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements - [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer - [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons - [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords - [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip - [#1475](#1475) [`6a218bcc1`](6a218bc) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - add non-visual text as a description for expression widget - Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Minor Changes - [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor - [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs - [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs - [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) ### Patch Changes - [#1491](#1491) [`395b44f29`](395b44f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Correct flag logic for polygon graph start coords UI - [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements - [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead - [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer - [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons - [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords - [#1471](#1471) [`b4e0b60dc`](b4e0b60) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the dropdowns overlapping labels in locked figures settings - [#1481](#1481) [`a35be719f`](a35be71) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Stop page scrolling on number text field focused scroll - [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip - [#1477](#1477) [`653b520c6`](653b520) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate ItemExtrasEditor to WB checkboxes - Updated dependencies \[[`a8aaac339`](a8aaac3), [`de13fd337`](de13fd3), [`8b0268215`](8b02682), [`2ebbc1978`](2ebbc19), [`6c6ff52f4`](6c6ff52), [`18f38fca4`](18f38fc), [`ef96cdd54`](ef96cdd), [`342a72211`](342a722), [`de2883b3f`](de2883b), [`7fd586ef4`](7fd586e), [`f920a4cc7`](f920a4c), [`eb9f3f9c0`](eb9f3f9), [`4c2ace57a`](4c2ace5), [`53031f8df`](53031f8), [`808956098`](8089560), [`5e66539e6`](5e66539), [`0b625f560`](0b625f5), [`6a218bcc1`](6a218bc), [`0bec013e8`](0bec013)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1495](#1495) [`6c6ff52f4`](6c6ff52) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove old buttons that we weren't using anymore - [#1475](#1475) [`342a72211`](342a722) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update wonder blocks popover versions - [#1496](#1496) [`5e66539e6`](5e66539) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove unused buttons from MathInput; add Lato ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: Myranae Required Reviewers: Approved By: Myranae Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1473
Summary:
KaTeX was retired from use in Perseus in 2023. Today, I noticed that we still reference the
katex
package 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 tsc
yarn lint
I ran storybook and opened the
ZoomableTex
stories and played with them.Note(jeremy): I've restored the code that swaps
\begin{align}
to\begin{aligned}
and noted LEMS-1608 based on comments from Ben (thanks!).I also went to theEditorPage
story 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}
).