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

chore(storybook): fix code loading on docs pages [CSS-1070] #3440

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

castastrophe
Copy link
Collaborator

@castastrophe castastrophe commented Dec 10, 2024

Description

The "Show code" feature is causing Storybook to freeze, which means that a user cannot view the reference markup on the documentation (without using the inspector).

Upon investigation, I determined this was caused by the testing grid injecting so much markup onto docs pages that it was overloading the code previewer. The testing code now returns only the template on docs pages when the Variant() function is used.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Open the storybook docs page for any component:

  • Click the "show code" button at the bottom of one of the examples; expect the code to display in a reasonable amount of time
  • Repeat for at least 2 other snippets on the page
  • Validate on Safari [@castastrophe]
  • Validate on Chrome
  • Validate on Firefox

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@castastrophe castastrophe added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action) storybook high priority An important PR or issue requiring immediate attention labels Dec 10, 2024
@castastrophe castastrophe self-assigned this Dec 10, 2024
Copy link

changeset-bot bot commented Dec 10, 2024

🦋 Changeset detected

Latest commit: e6f0e83

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/preview Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 10, 2024

🚀 Deployed on https://pr-3440--spectrum-css.netlify.app

@@ -40,7 +40,6 @@
"@etchteam/storybook-addon-status": "^5.0.0",
"@storybook/addon-a11y": "^8.4.6",
"@storybook/addon-actions": "^8.4.6",
"@storybook/addon-console": "^3.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This plugin adds little value and is pretty slow as well so I included here to ensure reasonable load times for pages.

@@ -51,7 +50,6 @@
"@storybook/core-events": "^8.4.6",
"@storybook/manager-api": "^8.4.6",
"@storybook/preview-api": "^8.4.6",
"@storybook/testing-library": "^0.2.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also unused so removed this to prevent any conflicting code loaded.

@@ -94,11 +81,6 @@ export const parameters = {
},
status: {
statuses: {
migrated: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unused code

useTabs: false,
htmlWhitespaceSensitivity: "ignore",
},
highlighter: {
showLineNumbers: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Show duplicate line numbers when this is set to true

Copy link
Contributor

github-actions bot commented Dec 10, 2024

File metrics

Summary

Total size: 4.27 MB*

🎉 No changes detected in any packages

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Member

@cdransf cdransf left a comment

Choose a reason for hiding this comment

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

I see a brief delay before the code loads after hitting Show code but it no longer freezes or locks up the application.

@rubencarvalho
Copy link

These changes reduced the amount of code in the "Show code" snippets. However, for components with large amounts of markup, they still render very slowly and can cause the page to become unresponsive.
If I try to:

  1. Visit https://pr-3440–spectrum-css.netlify.app/?path=/docs/components-table–docs.
  2. Click Show code under the first “Default variant” and wait for the snippet to render.
  3. Then click Show code under the “Collapsible” variant. At this point, the page becomes unresponsive.

Chrome 131.0.6778.86; Mac (M3 Max)
Screenshot 2024-12-10 at 16 41 35

Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

As Ruben just noted, it's still giving "Page Unresponsive" for many components (Chrome, Mac). Some ones I tested of note:

  • Accordion: will show the unresponsive message, but the code actually loads a few seconds later after that initial delay
  • Checkbox: unresponsive message but the code never loads and page fully locks up after waiting
  • Card and Button: code does load but there is still a bit of a delay

@castastrophe castastrophe force-pushed the chore-storybook-code-preview-blocking branch 3 times, most recently from 6237e13 to 7c8c3cc Compare December 10, 2024 20:37
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

This appears to be loading a lot faster now. I'm now able to load the Checkbox and Accordion code. Was it related to the icon loading and/or Typekit loading?

One significant question below about whether --test should be in there, otherwise I think it's good to go.

.storybook/project.json Outdated Show resolved Hide resolved
.storybook/preview-head.html Outdated Show resolved Hide resolved
.storybook/main.js Outdated Show resolved Hide resolved
.changeset/famous-comics-run.md Show resolved Hide resolved
@castastrophe castastrophe added run_vrt For use on PRs looking to kick off VRT and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Dec 12, 2024
@castastrophe castastrophe force-pushed the chore-storybook-code-preview-blocking branch 7 times, most recently from 7a3404a to 8655320 Compare December 16, 2024 14:17
@castastrophe castastrophe force-pushed the chore-storybook-code-preview-blocking branch 23 times, most recently from 365da61 to 96ac257 Compare December 17, 2024 00:38
@castastrophe castastrophe force-pushed the chore-storybook-code-preview-blocking branch 3 times, most recently from 1340b84 to a479cc4 Compare December 23, 2024 20:08
@castastrophe castastrophe force-pushed the chore-storybook-code-preview-blocking branch from a479cc4 to e6f0e83 Compare December 23, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority An important PR or issue requiring immediate attention run_vrt For use on PRs looking to kick off VRT size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show Code Button does not work on MS Edge and Chrome for code documentation.
5 participants