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

Storybook 7 #972

Merged
merged 2 commits into from
Jun 5, 2023
Merged

Storybook 7 #972

merged 2 commits into from
Jun 5, 2023

Conversation

nataliepina
Copy link
Collaborator

@nataliepina nataliepina commented May 25, 2023

Description

These changes introduce the migration from Storybook 6.5 to Storybook 7.

Issues Encountered

There was a breaking change for the Welcome page. Storybook's styles now will override all story files, including these MDX files. This broke our theming and styles for this page. I discovered a workaround in a community issue. We can use Unstyled to remove any storybook style overrides. 
Also, for theming to work correctly, it will now come from "@storybook/theming/create".

The favicon broke when upgrading to storybook 7. I chose a new method for us to integrate the favicon with the manager-preview.html file. The favicon won't appear locally but it will appear in the production build. You can test this out by running npx http-server storybook_static. The favicon as an svg is the more modern way to manage this.

The codemod took care of the migration of our stories from CSF2 to CSF3. There were a few stories that were broken and required tweaks. The decorators for the Toast component broke the Docs page. And you would see this error - Error: Rendered more hooks than during the previous render. I fixed this by refactoring the stories so that Toaster will wrap around the Toast component within the individual stories. It doesn't need to be a decorator.

There were a few other locations that had render errors. To fix this I had to slightly adjust the formatting of the story. PageHeader is an example of a story that I had to adjust. 
There also were some errors in the code due to props being overridden in stories. Infobox required a slight refactor to get the message to render.

Other Notable Changes

With these changes, I was also able to migrate us to MDX2! This only impacts the Welcome page, Typography, and Colors.

I wanted the changes to isolate the update to Storybook 7 primarily so that it’s easier to review. I did a side-by-side comparison of the component docs. I’ve noted a few locations where improvements could be made as a follow-up.

These changes have also been tested downstream.

Which issue(s) does this PR relate to?

Testing

  1. Make sure to run an npm install
  2. Build storybook npm run build:storybook
  3. Start Storybook npm run start
  4. Perform a thorough regression test.

Trade-offs

It takes a few seconds longer for storybook to start up. I believe we can improve this by refactoring our webpack configuration and reduce/refactor our stories. Switching between stories seems to be more performant though.

Screenshots

Screenshot 2023-05-30 at 12 29 26 PM

Checklist

  • This PR is associated with a JIRA and it is mentioned in the commit message footer ("Closes …")
  • Significant changes have been tested downstream to avoid breaking changes
  • This PR contains breaking changes and states in the commit message body ("BREAKING CHANGE: …")
  • I have reviewed the changes and provided detail to the sections above

@github-actions
Copy link

github-actions bot commented May 25, 2023

Uffizzi Preview deployment-26684 was deleted.

@nataliepina nataliepina force-pushed the npina/storybook-7 branch 9 times, most recently from 0f6cd11 to e3869e9 Compare May 30, 2023 18:04
@@ -8,9 +8,6 @@ module.exports = ({ config }) => {
config.resolve.extensions = config.resolve.extensions.concat(
webpackBase.getExtensions()
);
config.resolve.alias["core-js"] = path.dirname(require.resolve("core-js"));
config.module.rules[0].use[0].options.sourceType = "unambiguous";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These items are no longer required, and they will not allow Storybook to build.

@nataliepina nataliepina marked this pull request as ready for review May 30, 2023 19:01
@nataliepina nataliepina requested a review from a team as a code owner May 30, 2023 19:01
Copy link
Contributor

@clintonmedbery clintonmedbery left a comment

Choose a reason for hiding this comment

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

I didn't experience too much slow down in the running process. Thanks for doing this!

Copy link
Contributor

@seialkali seialkali left a comment

Choose a reason for hiding this comment

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

Looks good! Great work persevering with all the issues you came up against and finding solutions 💥

@nataliepina nataliepina merged commit e10af7c into main Jun 5, 2023
@nataliepina nataliepina deleted the npina/storybook-7 branch June 5, 2023 20:15
@github-actions
Copy link

🎉 This PR is included in version 14.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants