Skip to content
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

[DRAFT] New Perseus Preview component #1304

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

Conversation

jeremywiebe
Copy link
Collaborator

@jeremywiebe jeremywiebe commented May 22, 2024

Summary:

Today, the Perseus editor preview component uses a complex <iframe /> based system that depends on a live website providing the iframe source. This makes it challenging to mimic the preview system in Storybook and has its own set of issues in production.

Added to that, the mechanism to communicate between the Perseus Editor and the preview frame is not at all easy to understand and is split between this repo (Khan/perseus) and our main webapp repo. This makes maintaining the preview system tricky and error prone (basically, we don't touch it!)

This PR introduces a new preview component that simply renders the ServerItemRenderer using the current PerseusItem. This eliminates all of the cross-frame communication that was needed and seems to work just as good as the iframe system.

A few notes before we commit to this:

  • We need preview components that support hints and articles (either we make the preview component more flexible or render different preview components where they're needed - I think I'd lean towards the latter as I don't think there's a need to have One Master Preview Component To Rule Them All™️ )

  • The expression widget when simulating mobile can display the math keypad, but it doesn't automatically dismiss when the field is blurred. In testing, Nisha and I found that this is actually semi-broken in production anyways so this wouldn't be a regression.

  • The old iframe preview system uses a helper tool from the HammerJS library, which is vendored in, to simulate touches when previewing for Phone and Tablet. This isn't possible with a non-iframe preview system because that library "mutates" the global env, so we'd have to reload the page to switch between preview devices. I think a way forward might be to change the math keypad to support Pointer Events instead of being so touch-centric.

  • Linting: I haven't figured out exactly how this all works, but there is a LinterContextProps prop on some top-level editor components that seems involved. Also, the simple-markdown package is the place where we actually render the <Lint /> component that surfaces lint errors beside the preview. It seems like triggerPreviewUpdate() call in the EditorPage is related (here). You can see an example of a linter error and how it's shown in the screenshot below.

    image

    Linting is somewhat working! :) @mpolyak

    image

Issue: LEMS-1809

Test plan:

yarn start
Navigate to the Editor Page story and add any widgets. The preview should update in near realtime as you edit the content and widget configs.

@jeremywiebe jeremywiebe self-assigned this May 22, 2024
Copy link
Contributor

github-actions bot commented May 22, 2024

Size Change: +1.26 kB (+0.15%)

Total Size: 857 kB

Filename Size Change
packages/math-input/dist/es/index.js 78.2 kB -2.04 kB (-2.54%)
packages/math-input/dist/es/strings.js 1.79 kB +61 B (+3.53%)
packages/perseus-editor/dist/es/index.js 278 kB +3.27 kB (+1.19%)
packages/perseus/dist/es/index.js 414 kB -39 B (-0.01%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.3 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-linter/dist/es/index.js 21.6 kB
packages/perseus/dist/es/strings.js 3.29 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

@mpolyak
Copy link
Contributor

mpolyak commented May 23, 2024

Very cool, thank you for working on this!

In this approach of rendering ServerItemRenderer directly, would we still be able to highlight content lint warnings/errors in the editor?

@jeremywiebe
Copy link
Collaborator Author

Very cool, thank you for working on this!

In this approach of rendering ServerItemRenderer directly, would we still be able to highlight content lint warnings/errors in the editor?

Yes, linting, good call! I've added that to the PR description as something we'd need to figure out!

Base automatically changed from jer/remove-unused to main May 23, 2024 22:36
@jeremywiebe jeremywiebe force-pushed the jer/preview-no-iframe branch from 554b1d9 to 19fe692 Compare July 16, 2024 22:42
@jeremywiebe jeremywiebe force-pushed the jer/preview-no-iframe branch from 520bf66 to 39e40f1 Compare July 25, 2024 00:40
@jeremywiebe jeremywiebe marked this pull request as ready for review July 25, 2024 15:32
@jeremywiebe jeremywiebe changed the title New Perseus Preview component [DRAFT] New Perseus Preview component Jul 25, 2024
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Jul 25, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/hungry-seals-refuse.md, .storybook/preview-head.html, dev/vite.config.ts, packages/perseus-editor/tsconfig-build.json, packages/perseus/src/dependencies.ts, packages/perseus-editor/src/article-editor.tsx, packages/perseus-editor/src/editor-page.tsx, packages/perseus-editor/src/editor.tsx, packages/perseus-editor/src/hint-editor.tsx, packages/perseus-editor/src/iframe-renderer.tsx, packages/perseus-editor/src/index.ts, packages/perseus-editor/src/item-editor.tsx, packages/perseus/src/__testdata__/article-renderer.testdata.ts, packages/perseus/src/components/lint.tsx, packages/perseus-editor/src/__stories__/article-editor.stories.tsx, packages/perseus-editor/src/__stories__/editor-page-with-storybook-preview.tsx, packages/perseus-editor/src/__stories__/editor-page.stories.tsx, packages/perseus-editor/src/__stories__/iframe-renderer.stories.tsx, packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx, packages/perseus-editor/src/components/device-framer.tsx, packages/perseus-editor/src/components/viewport-resizer.tsx, packages/perseus-editor/src/preview/content-renderer.tsx, packages/perseus-editor/src/preview/iframe-content-renderer.tsx, packages/perseus-editor/src/widgets/group-editor.tsx, packages/perseus/src/components/__stories__/lint.stories.tsx, packages/perseus-editor/src/preview/__stories__/content-renderer.stories.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@khan-actions-bot khan-actions-bot requested a review from a team July 25, 2024 15:32
Copy link
Contributor

github-actions bot commented Jul 25, 2024

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (143f88c) and published it to npm. You
can install it using the tag PR1304.

Example:

yarn add @khanacademy/perseus@PR1304

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1304

@jeremywiebe jeremywiebe force-pushed the jer/preview-no-iframe branch 4 times, most recently from f200ffd to 6a2f498 Compare August 8, 2024 16:47
@jeremywiebe jeremywiebe force-pushed the jer/preview-no-iframe branch from 6a2f498 to 74816d7 Compare August 12, 2024 23:17
@jeremywiebe jeremywiebe force-pushed the jer/preview-no-iframe branch from be253d4 to a80c35b Compare August 13, 2024 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants