Skip to content

Proposal for an improved EditorPageWithStorybookPreview to better mock actual implementation that uses iFrames #2543

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
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SonicScrewdriver
Copy link
Contributor

@SonicScrewdriver SonicScrewdriver commented May 30, 2025

Summary:

[neverending-story-2] New Editor Page that properly implements iframe communication

NOTE: THIS IS A DRAFT PR and still needs polish / cleanup.

Copy link
Contributor

github-actions bot commented May 30, 2025

Size Change: 0 B

Total Size: 481 kB

ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 20.7 kB
packages/keypad-context/dist/es/index.js 1 kB
packages/kmath/dist/es/index.js 5.98 kB
packages/math-input/dist/es/index.js 98.6 kB
packages/math-input/dist/es/strings.js 1.61 kB
packages/perseus-core/dist/es/index.js 21.2 kB
packages/perseus-editor/dist/es/index.js 91.3 kB
packages/perseus-linter/dist/es/index.js 7.14 kB
packages/perseus-score/dist/es/index.js 9.25 kB
packages/perseus-utils/dist/es/index.js 403 B
packages/perseus/dist/es/index.js 208 kB
packages/perseus/dist/es/strings.js 7.56 kB
packages/pure-markdown/dist/es/index.js 1.22 kB
packages/simple-markdown/dist/es/index.js 6.72 kB

compressed-size-action

const onChangeAction = action("onChange");

function EditorPageWithStorybookPreview(props: Props) {
const [previewDevice, setPreviewDevice] =
React.useState<DeviceType>("phone");
React.useState<DeviceType>("desktop");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upstream defaults to desktop, so I figured we should too.

Copy link
Contributor

github-actions bot commented May 30, 2025

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2543

If you are working in Khan Academy's frontend, you can run the below command.

./tools/bump_perseus_version.ts -t PR2543

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR2543

@SonicScrewdriver SonicScrewdriver changed the title docs(changeset): Overhaul of EditorPageWithStorybookPreview to better mock actual implementation docs(changeset): Overhaul of EditorPageWithStorybookPreview to better mock actual implementation using iFrames May 30, 2025
@SonicScrewdriver SonicScrewdriver changed the title docs(changeset): Overhaul of EditorPageWithStorybookPreview to better mock actual implementation using iFrames Proposal for an improved EditorPageWithStorybookPreview to better mock actual implementation that uses iFrames May 31, 2025

const apiOptions = props.apiOptions ?? {
isMobile: false,
const isMobile = previewDevice === "phone" || previewDevice === "tablet";
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@SonicScrewdriver
Copy link
Contributor Author

There's a failing linter here, but I want to confirm with the team about whether and/or where we should be declaring the globals before fixing it!

interface Window {
iframeDataStore: Record<string, any>;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporary location while I determine the right location for this, if there is one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the right spot is to get rid of this as a way to send data between the host page and the iframe. Ideally, this should pass the data along with the postMessage() call. Not asking you to do that now, but mid-term, that'd be how we fix this.

Copy link
Collaborator

@jeremywiebe jeremywiebe left a comment

Choose a reason for hiding this comment

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

Love that you've started into this! I have some comments but generally am very happy to see you pushing into cleaning this up and enabling it in Storybook!

interface Window {
iframeDataStore: Record<string, any>;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the right spot is to get rid of this as a way to send data between the host page and the iframe. Ideally, this should pass the data along with the postMessage() call. Not asking you to do that now, but mid-term, that'd be how we fix this.

Comment on lines +71 to +72
frameSource="/iframe.html?id=perseuseditor-perseus-frame--frame&viewMode=story"
previewURL="/iframe.html?id=perseuseditor-perseus-frame--frame&viewMode=story"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this leverage a Storybook iframe feature? I'd be mildly concerned that this could break if someone re-names a story. We could also just put a container page in the Vite static asset directory if you think that's more resilient to changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't need to leverage much of storybook at all! I tried to avoid too much storybook middleware as it seemed to diverge a little from how we would set it up in production.

This currently links to the Perseus Frame Component's story in "story" mode (AKA the full page view). I think the vite static asset directory could make a lot of sense, as it's definitely possible for this to break if someone moves the Perseus Frame component story. While I don't think it's likely that someone will rename the story, I think the static asset directory is a good way to keep us safe. :) I'll look into getting that working!

Comment on lines +91 to 151
export const EditorWithDropdownWidget: Story = {
name: "Editor with Dropdown Widget",
args: {
question: {
content:
"Which of the following is a prime number?\n\n[[☃ dropdown 1]]",
images: {},
widgets: {
"dropdown 1": {
type: "dropdown",
graded: true,
options: {
choices: [
{
content: "Choose an answer",
correct: false,
},
{
content: "4",
correct: false,
},
{
content: "6",
correct: false,
},
{
content: "7",
correct: true,
},
{
content: "8",
correct: false,
},
],
placeholder: "Choose an answer",
static: false,
},
version: {
major: 0,
minor: 0,
},
},
},
},
hints: [
{
content:
"A prime number is a number greater than 1 that has no positive divisors other than 1 and itself.",
images: {},
widgets: {},
},
],
},
parameters: {
docs: {
description: {
story: "Editor with dropdown widget to test preview frame communication and height adjustments.",
},
},
},
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can also get documentation for a story by putting a JSDoc comment on the story itself. It's a little more compact and, I think, more natural to find for future maintenance.

Suggested change
export const EditorWithDropdownWidget: Story = {
name: "Editor with Dropdown Widget",
args: {
question: {
content:
"Which of the following is a prime number?\n\n[[☃ dropdown 1]]",
images: {},
widgets: {
"dropdown 1": {
type: "dropdown",
graded: true,
options: {
choices: [
{
content: "Choose an answer",
correct: false,
},
{
content: "4",
correct: false,
},
{
content: "6",
correct: false,
},
{
content: "7",
correct: true,
},
{
content: "8",
correct: false,
},
],
placeholder: "Choose an answer",
static: false,
},
version: {
major: 0,
minor: 0,
},
},
},
},
hints: [
{
content:
"A prime number is a number greater than 1 that has no positive divisors other than 1 and itself.",
images: {},
widgets: {},
},
],
},
parameters: {
docs: {
description: {
story: "Editor with dropdown widget to test preview frame communication and height adjustments.",
},
},
},
};
/**
* Editor with dropdown widget to test preview frame communication and
* height adjustments.
*/
export const EditorWithDropdownWidget: Story = {
name: "Editor with Dropdown Widget",
args: {
question: {
content:
"Which of the following is a prime number?\n\n[[☃ dropdown 1]]",
images: {},
widgets: {
"dropdown 1": {
type: "dropdown",
graded: true,
options: {
choices: [
{
content: "Choose an answer",
correct: false,
},
{
content: "4",
correct: false,
},
{
content: "6",
correct: false,
},
{
content: "7",
correct: true,
},
{
content: "8",
correct: false,
},
],
placeholder: "Choose an answer",
static: false,
},
version: {
major: 0,
minor: 0,
},
},
},
},
hints: [
{
content:
"A prime number is a number greater than 1 that has no positive divisors other than 1 and itself.",
images: {},
widgets: {},
},
],
},
};

docs: {
description: {
component:
"Perseus EditorPage with built-in preview functionality using Storybook iframe. ",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this description show up when it's done on the Meta object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh weird. These were showing up locally for me on a "docs" page added for the relevant folders, but they no longer do after rebasing the newest changes from main into this branch. Did we turn off the docs feature when upgrading Storybook?

You can see the docs on the storybook publish for this PR:
https://650db21c3f5d1b2f13c02952-ejozbvanyy.chromatic.com/?path=/docs/perseuseditor-perseus-frame--docs
https://650db21c3f5d1b2f13c02952-ejozbvanyy.chromatic.com/?path=/docs/perseuseditor-editorpage--docs

Here's one we had from before:
https://650db21c3f5d1b2f13c02952-ejozbvanyy.chromatic.com/?path=/docs/perseus-components-number-input--docs

Copy link
Collaborator

Choose a reason for hiding this comment

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

We didn't turn it off as far as I know, but I did upgrade us to Storybook v9 and I wonder if that changed things! I'll look into that. Fix: #2565

However, I still think it'd be better to use JSDoc comments as suggested.. they show up the same way and (IMHO at least) are much more natural than figuring out how to add the right combination of docs parameters etc. :)

See: https://storybook.js.org/docs/api/doc-blocks/doc-block-description#writing-descriptions

*
* @returns A PerseusFrameComponent that renders the content of the EditorPage.
*/
const PerseusFrameComponent = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fantastic that you reverse-engineered this info. Ideally, I'd love to extract the same component out of webapp and move it into Perseus (so that we'd have both sides of the communication channel in Perseus) and then eventually refactor to use the same component in the stories as in production. Future work, but this definitely paves the way! Thanks.

jeremywiebe added a commit that referenced this pull request Jun 9, 2025
## Summary:

@third [noticed](#2543 (comment)) that our Storybook docs weren't working anymore. This is a regression that happened during the Storybook v9 upgrade. 

This PR fixes autodocs (and enables the TOC feature which is a nice little addition). 


Issue: "none"

## Test plan:

`pnpm install; pnpm start` -> stories have a "Docs" entry again

Author: jeremywiebe

Reviewers: SonicScrewdriver

Required Reviewers:

Approved By: SonicScrewdriver

Checks: ✅ 8 checks were successful

Pull Request URL: #2565
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants