Added MathJax rendering #4

dat-boris · 2023-08-11T23:29:03Z

Summary:

With the recent need for Math rendering in Khan Academy, we need to pick up the MathJax
rendering again.

This added MathJax rendering to the conversation whenever we detect any
conversation that contains MathJax inline in the conversation.

Note we need to also consider the slot filling features - we do this by setting an attribute
(data-skip-node) which when met this will allow dynamic MathJax rendering to happen
within the node, without affecting the highlight of the elements.

Issue: https://khanacademy.atlassian.net/browse/DI-533

Test plan:

yarn test

yarn start and see the equation rendered.

Note that we also added the slots to demonstrate different granularity of the tags

With the highlight slot (unfortunately we cannot highlight the MathJax expression)

And also test out dynamic rendering by using our local label studio repo with multiiple entries.

E2E tests

Although the official E2E test is broken (it seems like it depends on some external resources on some audio test which times out) - We can manaully run those E2E tests manually to confirm that the rich-text component is not borken.

% npx codeceptjs run tests/rich-text/*.js                              
CodeceptJS v3.3.3 #StandWithUkraine
Using test root "/Users/borislau/khan/label-studio-frontend/e2e"

Richtext basic functional --
Warning: Timeout was set to 2400secs.
Global timeout should be specified in seconds.
  ✔ Creating, removing and restoring regions in 8394ms
  ✔ Rich text content consistency in 25356ms

  OK  | 2 passed   // 35s

Previously although RichTextTable display properly, the annotaiton didn't work right. This PR is a result of a bunch of learning on how the registry works, and realiase a few small things that we did wrong and need to hook up properly. These will be documented in our documentation [1]. [1] https://khanacademy.atlassian.net/wiki/spaces/INFRA/pages/2373714068/Label+Studio+Frontend+guide Issue: https://khanacademy.atlassian.net/browse/DI-533 Test plan: - `yarn start` and goes to https://localhost:3000, and try out the annotation.

src/tags/object/RichText/table.js

dat-boris · 2023-08-11T23:37:12Z

Ah ha, love the linter output - I will fix that next week (the import is already fixed in #3)

This added MathJax rendering to the conversation whenever we detect any conversation that contains MathJax inline in the conversation. Note this will interact with highlighting annotation. Instead of making the annotation code much more complex, we just display a seperate formula rendering below the conversation (see screenshot). This seems to be the easiest soluation to annotate and without losing conversation context. Issue: https://khanacademy.atlassian.net/browse/DI-533 Test plan: `yarn start` and experiment with annotating the expressions

dat-boris

The overall code should be set, but highlighting some of the to fix here.

src/tags/object/RichText/table.js

dat-boris · 2023-08-14T18:14:52Z

src/tags/object/RichText/table.js

+    };
+
+    return (
+      <MathJaxContext config={mathJaxConfig}>


This needs fixing, looks like I need to set the dynamic flag in the config

https://github.com/fast-reflexes/better-react-mathjax#dynamic-boolean--undefined

The dynamic flag seems to be for MathJax content changing. However, it does not seems to trigger a new component to typeset (i.e. render into Latex), which is what we want. I added a hacky way to drill a function into ComponentDidMount, which works good enough for now.

https://github.com/fast-reflexes/better-react-mathjax#rendermode-pre--post--undefined
I was skimming the docs for the mathjax stuff since I'm totally unfamiliar with it, would setting the rendermode to post fix this? sounds like it causes the typeset to process on every render, which would be what we want here, right? I barely speak React, so maybe i'm way off here, or just not understanding the problem space.

Ha picking this up after almost a year!

Thanks for the suggestions - I agree it sounds promising, but tried the renderMode="pre" together with combination of <MathJax text="...."/>, and it didnt work. I didn't dig too deeply since the ComponentDidMount solution is working okay for now.

jimmykodes

To the extent I understand react, this looks good. one Q in the comments, but approving so as not to block

jimmykodes · 2023-08-15T14:12:51Z

src/tags/object/RichText/table.js

+    };
+
+    return (
+      <MathJaxContext config={mathJaxConfig}>


https://github.com/fast-reflexes/better-react-mathjax#rendermode-pre--post--undefined
I was skimming the docs for the mathjax stuff since I'm totally unfamiliar with it, would setting the rendermode to post fix this? sounds like it causes the typeset to process on every render, which would be what we want here, right? I barely speak React, so maybe i'm way off here, or just not understanding the problem space.

wwells

+1 to Jimmy's note around react as a foreign language.

Have a question about the benefit - forgive my lack of understanding. What triggers the rendering? Is it automatic or does the labeling coordinator have to specify something in the schema that will give the option of rendering. My limited understanding is that it's automatic.

If that's correct, I'm a bit worried that while this will be necessary and useful for workstreams like better-math, since we aren't scraping out the rendered math from the transcript, I worry a bit about how this changes the UX for a user of something like bad-links...

dat-boris · 2023-08-15T17:02:11Z

Turn out that this actually interfere with Math annotation unfortunately. So I'm going to keep this open and fix this in future ticket (https://khanacademy.atlassian.net/browse/DI-660).

I was skimming the docs for the mathjax stuff since I'm totally unfamiliar with it, would setting the rendermode to post fix this? sounds like it causes the typeset to process on every render, which would be what we want here, right? I barely speak React, so maybe i'm way off here, or just not understanding the problem space.

Good suggestion! I will try that.... the issue I feel is less with react, but more with the MathJax interaction which I probably need to dig into how better-react-mathjax does that.

If that's correct, I'm a bit worried that while this will be necessary and useful for workstreams like better-math, since we aren't scraping out the rendered math from the transcript, I worry a bit about how this changes the UX for a user of something like bad-links...

Good question! The rendering should only happen with has_math flag is detected, so it should only affect conversation that has math that uses this component. But good idea that maybe we should explicitly enable this in the component to avoid noise.

src/tags/object/RichText/table.js

src/utils/selection-tools.js

src/tags/object/RichText/table.js

Create a new FE build to include new mathjax. Khan/label-studio-frontend#4

dat-boris · 2024-07-22T23:44:27Z

Okay, this is now ready for review. Internally, this PR is now deployed on our staging server, which can be tested to confirm that it is rendering.

Note that the E2E test is broken on Github - that is due to some external depdnency on resource 404 that makes the E2E test hang and timeout. I'm manually run the E2E test (see test plan) and confrm it is working.

(cc @lizfaubell )

wwells

Thanks so much for working on these improvements! I'm definitely not qualified to review React/JS, but what I can grok seems reasonable.

Skimming some of the test projects, it seems the rendering integrity is hit or miss?

lizfaubell

Thank you!!! This makes math so much easier to read in threads.

lizfaubell · 2024-07-23T20:39:43Z

src/tags/object/RichText/table.js

+// However we also want to keep the original \(expressions\), so we swap into
+// a differnet marker instead.
+// We use a different marker than what is used in Khanmigo.
+const MAJX_MARKER = '$';


Minor: should this be MATHJX_MARKER

lizfaubell · 2024-07-23T20:41:49Z

src/tags/object/RichText/table.js

+        if (i % 2 === 0) {
+          // Non math
+          return <span key={`eq=${i}`}>{convo}</span>;


Sorry I think I'm missing something - how come every other one is not math?

Should explain this better in comment - parseConvoWithMath return a list of Math / non-math expression alternatively, which we wraps around here.

…3044c95489edb175be2bcd6a5529a38f8377 at Khan/label-studio-frontend#4 Workflow run: https://github.com/Khan/label-studio/actions/runs/10066365565

dat-boris added 10 commits August 10, 2023 08:03

Fix up label selector to ensure region is editable

c862d05

Fix up missing modules

f5f8afa

Remove old debugging code

ebba48c

Added better comments

e23407e

Merge branch 'master' into fix_annotation

4a6bef4

Guess who need spellcheck... push to retrigger test

ea17216

Fix bug caught by e2e/tests/date-time.test.js

95cf825

Disable functional tests

11f8f6d

Disable flaky e2e tests

7df51b4

dat-boris self-assigned this Aug 11, 2023

dat-boris marked this pull request as ready for review August 11, 2023 23:34

dat-boris requested review from wwells, jimmykodes and lizfaubell August 11, 2023 23:34

github-actions bot reviewed Aug 11, 2023

View reviewed changes

src/tags/object/RichText/table.js Outdated Show resolved Hide resolved

src/tags/object/RichText/table.js Outdated Show resolved Hide resolved

src/tags/object/RichText/table.js Outdated Show resolved Hide resolved

dat-boris added 2 commits August 11, 2023 22:43

Fix up hasMath variable

9c795cd

dat-boris commented Aug 14, 2023

View reviewed changes

Merge branch 'master' into mathjax-comp

9d933cd

dat-boris force-pushed the mathjax-comp branch from bd27d7a to 9d933cd Compare August 14, 2023 19:33

dat-boris added 3 commits August 14, 2023 14:12

Merge branch 'merge_1.8.2' into mathjax-comp

b2c972e

Merge branch 'master' into mathjax-comp

4ca1d21

Allow render multiple regex

f785fb5

jimmykodes approved these changes Aug 15, 2023

View reviewed changes

wwells approved these changes Aug 15, 2023

View reviewed changes

Added dynamic component loading

b1959dd

dat-boris force-pushed the mathjax-comp branch from f7bc9df to b1959dd Compare July 16, 2024 22:24

github-actions bot reviewed Jul 16, 2024

View reviewed changes

dat-boris added 3 commits July 16, 2024 16:49

Remove unnecessary classes and firefox fix

98b61bc

lint

827c7b1

Merge branch 'master' into mathjax-comp

4dfeb56

github-actions bot reviewed Jul 17, 2024

View reviewed changes

src/tags/object/RichText/table.js Outdated Show resolved Hide resolved

dat-boris added 2 commits July 16, 2024 17:05

relint

6e50a48

Avoid empty elements

2f94afa

dat-boris added a commit to Khan/label-studio that referenced this pull request Jul 17, 2024

FE build with Mathjax (tmp)

7fa4f43

Create a new FE build to include new mathjax. Khan/label-studio-frontend#4

dat-boris added a commit to Khan/label-studio that referenced this pull request Jul 17, 2024

FE build with Mathjax (tmp)

c8514fa

Create a new FE build to include new mathjax. Khan/label-studio-frontend#4

dat-boris added 2 commits July 18, 2024 18:08

Ensure that we only run one instance of typeset

bce4b11

Added fix for e2e tests

c787d5a

wwells approved these changes Jul 23, 2024

View reviewed changes

lizfaubell approved these changes Jul 23, 2024

View reviewed changes

Address comments

d03c36b

dat-boris merged commit ab88304 into master Jul 23, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added MathJax rendering #4

Added MathJax rendering #4

dat-boris commented Aug 11, 2023 •

edited

Loading

dat-boris commented Aug 11, 2023

dat-boris left a comment

dat-boris Aug 14, 2023

dat-boris Aug 15, 2023

jimmykodes Aug 15, 2023

dat-boris Jul 16, 2024

jimmykodes left a comment

jimmykodes Aug 15, 2023

wwells left a comment

dat-boris commented Aug 15, 2023 •

edited

Loading

dat-boris commented Jul 22, 2024 •

edited

Loading

wwells left a comment

lizfaubell left a comment

lizfaubell Jul 23, 2024

lizfaubell Jul 23, 2024

dat-boris Jul 23, 2024

Added MathJax rendering #4

Added MathJax rendering #4

Conversation

dat-boris commented Aug 11, 2023 • edited Loading

Summary:

Test plan:

E2E tests

dat-boris commented Aug 11, 2023

dat-boris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmykodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwells left a comment

Choose a reason for hiding this comment

dat-boris commented Aug 15, 2023 • edited Loading

dat-boris commented Jul 22, 2024 • edited Loading

wwells left a comment

Choose a reason for hiding this comment

lizfaubell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dat-boris commented Aug 11, 2023 •

edited

Loading

dat-boris commented Aug 15, 2023 •

edited

Loading

dat-boris commented Jul 22, 2024 •

edited

Loading