[Storybook] Update styles to be more accurate to prod #1945

nishasy · 2024-12-04T02:01:52Z

Summary:

Perseus Storybook in its current state does not accurately
reflect how these component look in prod.

A couple things are added here to fix this:

Stories are wrapped with the framework-perseus class.
Webapp styles are imported

box-sizing: border-box in particular affects all the
spacing (margins and padding)

Issue: none

Test plan:

Go to http://localhost:6006/?path=/docs/perseus-components-number-input--docs
Confirm that the inputs have rounded bordersa, and the
"mini", "small", and "normal" stories show different sizes.
Go to http://localhost:6006/?path=/story/perseuseditor-widgets-interactive-graph--interactive-graph-ray
Scroll to the start coordinates panels
Use the dev tools to confirm that the width is 308px
(previously 336px)

Screenshots:

Before	After

Perseus Storybook in its current state does not accurately reflect how these component look in prod. A couple things are added here to fix this: 1. Stories are wrapped with the `framework-perseus` class. 2. Webapp styles are imported - `box-sizing: border-box` in particular affects all the spacing (margins and padding) Issue: none Test plan: - Go to http://localhost:6006/?path=/docs/perseus-components-number-input--docs - Confirm that the inputs have rounded bordersa, and the "mini", "small", and "normal" stories show different sizes. - Go to http://localhost:6006/?path=/story/perseuseditor-widgets-interactive-graph--interactive-graph-ray - Scroll to the start coordinates panels - Use the dev tools to confirm that the width is 308px (previously 336px)

github-actions · 2024-12-04T02:04:31Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1945

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

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

github-actions · 2024-12-04T02:04:41Z

Size Change: 0 B

Total Size: 1.29 MB

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	697 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/index.js`	425 kB
`packages/perseus/dist/es/strings.js`	3.7 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

khan-actions-bot · 2024-12-04T02:53:22Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/shiny-sloths-obey.md, .storybook/preview.tsx, packages/perseus/src/styles/global.less, packages/perseus/src/styles/perseus-renderer.less, packages/perseus/src/styles/reset.css

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

jeremywiebe

I'm going to hold off approving. I'd love to find a way to set these styles as part of the Perseus bundle we deliver instead of mimicking things in Storybook.

jeremywiebe · 2024-12-04T16:18:18Z

.storybook/story-wrapper.tsx

+/**
+ * Wrapper for Storybook stories that
+ * 1. Renders the story inside of a .framework-perseus container
+ * 2. Includes the global styles from webapp


I would love it if we could isolate Perseus from these types of resets that trickle down into Perseus from outside. It makes sense to try to align styling with that of our primary consumer (webapp), but, if possible, could we make these styles/resets explicit in our Perseus styling system instead of mimicking them in Storybook?

Where would you propose adding this styles? We could do Renderer, but I don't think that would apply to our editors.

jeremywiebe · 2024-12-04T16:19:15Z

.storybook/webapp-styles.less

@@ -0,0 +1,136 @@
+/**
+ * This file contains styles that are used in webapp that ultimately


I'd like to avoid, if possible, references to projects outside of Perseus. I realize that might be pedantic, but Perseus is open source and although webapp is our primary consumer, I think it's worthwhile to stay at least somewhat agnostic of where Perseus is used.

Also, as stated in story-wrapper.tsx, I'm loathe to copy this much styling from an outside consumer just in Storybook. It gives us a false sense of what final styling will be in any given location if we've duplicated the styles only in Storybook, but not in our production styles.

Great points. I'll remove the reference to webapp. Maybe I can just say "prod"?

The other thing I was thinking of doing is just including the box-sizing: border-box style since that's where most of the spacing discrepancy is coming from. I added the other styles in case I missed something else, but I think it should be fine without those.

jeremywiebe · 2024-12-04T22:14:17Z

.storybook/global.less

I think this is a great place to put this. Then you can include it in perseus-renderer.less. That file is imported in renderer.tsx (which is our core renderer the all articles and exercises use). That should guarantee that these global styles are always included.

@jeremywiebe but maybe not in the .storybook folder, right? I can move it where perseus-renderer.less is?

Correct. This will become a part of our production Perseus CSS so it should go alongside the perseus-renderer.less file.

mark-fitzgerald · 2024-12-04T22:43:30Z

.storybook/global.less

+@wonderBlocksFontFamily: "Lato", "Noto Sans", "Helvetica", "Corbel", sans-serif;
+
+// Layers
+@layer reset, shared;


I think this needs to include legacy. The rest of our PROD site show:

@layer reset, shared, legacy;

That's true, but I left it out since @legacy isn't defined anywhere in perseus styles. Should I add it anyway?

Yes, please add it. Although legacy is last in the list, and "shouldn't" affect anything when imported due to definition order, we should be consistent with how it is declared in other parts in PROD.

That said, since this is the first time that we have declared and used layers in Perseus, we should verify how this comes across in PROD. Perhaps a ZND would help? I want to make sure that we aren't making sub-layers due to how these styles are imported.

mark-fitzgerald · 2024-12-04T22:46:21Z

.storybook/global.less

+        margin: 0;
+        color: @offBlack;
+        line-height: 1.4;
+        min-width: 0 !important;


Does this really need !important?

Ah, that was already there in webapp with the comment

/* Needs !important because we might load shared.css later */

But I don't think that's relevant anymore? I don't see a shared.css anywhere. I'll remove the !important.

mark-fitzgerald · 2024-12-04T22:49:41Z

.storybook/story-wrapper.tsx

+    return (
+        <div className="framework-perseus box-sizing-border-box-reset">
+            {props.children}
+        </div>


Do we need a separate file for this? Could we not just wrap the preview.tsx content in this <div> directly?

Ah yeah. Now that I removed the import, I'll change it back to how it was before.

mark-fitzgerald

I like the call-outs that Jeremy made. I added a couple additional items, but all else looks good.

jeremywiebe

Thanks for doing this Nisha. Hoping this will reduce inconsistencies between Storybook and prod even further. 👍

nishasy self-assigned this Dec 4, 2024

nishasy requested review from jeremywiebe, a team and benchristel December 4, 2024 02:02

prettier

d498113

nishasy marked this pull request as ready for review December 4, 2024 02:53

jeremywiebe reviewed Dec 4, 2024

View reviewed changes

Update comments, remove extra styles

281b86c

khan-actions-bot requested a review from a team December 4, 2024 19:39

prettier

1d9edc2

jeremywiebe reviewed Dec 4, 2024

View reviewed changes

mark-fitzgerald reviewed Dec 4, 2024

View reviewed changes

mark-fitzgerald approved these changes Dec 4, 2024

View reviewed changes

move styles out of storybook folder. remove unnecessary

0029d19

jeremywiebe approved these changes Dec 4, 2024

View reviewed changes

nishasy added 3 commits December 4, 2024 15:53

remove StoryWrapper. add legacy to layers

dd756f0

Merge branch 'main' into storybook-styles

30970dd

changeset

8766cc7

nishasy merged commit e69ca31 into main Dec 5, 2024
9 checks passed

nishasy deleted the storybook-styles branch December 5, 2024 00:01

khan-actions-bot mentioned this pull request Dec 5, 2024

Version Packages #1944

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storybook] Update styles to be more accurate to prod #1945

[Storybook] Update styles to be more accurate to prod #1945

nishasy commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

khan-actions-bot commented Dec 4, 2024 •

edited

Loading

jeremywiebe left a comment

jeremywiebe Dec 4, 2024

nishasy Dec 4, 2024

jeremywiebe Dec 4, 2024

nishasy Dec 4, 2024

jeremywiebe Dec 4, 2024

nishasy Dec 4, 2024

jeremywiebe Dec 4, 2024

mark-fitzgerald Dec 4, 2024

nishasy Dec 4, 2024

mark-fitzgerald Dec 4, 2024

mark-fitzgerald Dec 4, 2024

nishasy Dec 4, 2024

mark-fitzgerald Dec 4, 2024

nishasy Dec 4, 2024

mark-fitzgerald left a comment

jeremywiebe left a comment

		@@ -0,0 +1,136 @@
		/**
		* This file contains styles that are used in webapp that ultimately

[Storybook] Update styles to be more accurate to prod #1945

[Storybook] Update styles to be more accurate to prod #1945

Conversation

nishasy commented Dec 4, 2024 • edited Loading

Summary:

Test plan:

Screenshots:

github-actions bot commented Dec 4, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Dec 4, 2024 • edited Loading

khan-actions-bot commented Dec 4, 2024 • edited Loading

Gerald

jeremywiebe 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

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

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

mark-fitzgerald left a comment

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

nishasy commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

khan-actions-bot commented Dec 4, 2024 •

edited

Loading