Storybook: Upgrade to v8.0.x #67574
Open
Storybook: Upgrade to v8.0.x #67574
+1,279
−1,627
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.83 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 873b8bd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12194585165
|
This was referenced Dec 4, 2024
ciampo
reviewed
Dec 4, 2024
There was a problem hiding this comment.
What's the reason for this patch? Is it necessary to migrate to v8 ?
There was a problem hiding this comment.
Yeah, we need this patch primarily for two reasons:
Iconsfrom@storybook/componentsis deprecated and throws warnings@storybook/addonsimports no longer work and Storybook errors.
Support for v8 is in progress but not there yet, so we need the patch to unblock the upgrade temporarily. Hopefully, the v8 support will land soon. And maybe we can lend a hand if that doesn't happen for some reason.
This was referenced Dec 4, 2024
tyxla
force-pushed
the
try/storybook-v8
branch
3 times, most recently
from
e1bf894 to
81f7cf9
Compare
tyxla
force-pushed
the
try/storybook-v8
branch
2 times, most recently
from
bc1130f to
873b8bd
Compare
tyxla
commented
Dec 6, 2024
| @@ -33,7 +33,7 @@ | |||
| "@emotion/babel-plugin": "11.11.0", | |||
| "@emotion/jest": "11.7.1", | |||
| "@emotion/native": "11.0.0", | |||
| "@geometricpanda/storybook-addon-badges": "2.0.1", | |||
| "@geometricpanda/storybook-addon-badges": "2.0.5", | |||
There was a problem hiding this comment.
Needs to be bumped to support v8
| "@storybook/addon-docs": "8.0.10", | ||
| "@storybook/addon-toolbars": "8.0.10", | ||
| "@storybook/addon-viewport": "8.0.10", | ||
| "@storybook/addon-webpack5-compiler-babel": "3.0.3", |
There was a problem hiding this comment.
Now necessary in order to specifically use Babel, see https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#removed-babelcore-and-babel-loader-from-storybookbuilder-webpack5
| "@storybook/react": "8.0.10", | ||
| "@storybook/react-webpack5": "8.0.10", | ||
| "@storybook/source-loader": "8.0.10", | ||
| "@storybook/test": "8.0.10", |
There was a problem hiding this comment.
Used to explicitly declare action handler spies, see: https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#implicit-actions-can-not-be-used-during-rendering-for-example-in-the-play-function
Comment on lines
+48
to
+50
| args: { | ||
| onChange: fn(), | ||
| }, |
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't think that adding these lines of code is necessary since we're already specifying argTypesRegex above — I tested removing these lines on my machine and I was still getting "actions" firing correctly. This also matches the documentation about actions.
Given that I don't think we any Storybook tests, and that we typically use actions.argTypesRegex, I'm not sure we even need to list @storybook/test as a dependency for the time being?
If instead we wanted to change approach and go down the "explicit action spies" route, we'd need to refactor all stories specifying action or actions in the meta config object.
There was a problem hiding this comment.
Unfortunately, we seem to need these, otherwise I'm getting an error when navigating to these 3 stories (I tested them all, one by one, manually):
- http://localhost:50240/?path=/docs/components-customselectcontrol-v2--docs
- http://localhost:50240/?path=/docs/components-tabpanel--docs
- http://localhost:50240/?path=/docs/components-tabs--docs
Here's the error:
Once I add the action spies, the errors disappear.
There was a problem hiding this comment.
Weird. It doesn't make sense to me, since I would then expect this error to appear on a lot more stories. I think the checks a bit buggy (and we may not be the only ones).
For the sake of moving forward with the refactor, I'm OK keeping those changes and investigate the reason for the error in a separate follow-up PR
There was a problem hiding this comment.
I agree, and yep, would like to take a look separately.
Opened an issue here: #67834
Comment on lines
+26
to
+28
| args: { | ||
| onSelect: fn(), | ||
| }, |
There was a problem hiding this comment.
| @@ -114,8 +114,7 @@ export const parameters = { | |||
| <Subtitle /> | |||
| <Primary /> | |||
| <Description /> | |||
| { /* `story="^"` enables Controls for the primary props table */ } | |||
| <ArgsTable story="^" /> | |||
| <Controls /> | |||
There was a problem hiding this comment.
ArgsTable was removed in favor of Controls and ArgTypes, see https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#argstable-doc-block-removed
| @@ -28,7 +28,9 @@ const Title = styled.span( { | |||
|
|
|||
| const Icons = styled.span( {} ); | |||
|
|
|||
| const Icon = styled.span( {} ); | |||
| const Icon = styled.span( { | |||
| lineHeight: 1, | |||
There was a problem hiding this comment.
Needed to better vertically center the icon in the respective menu item.
| @@ -41,7 +43,7 @@ function useIcons( item ) { | |||
| return useMemo( () => { | |||
| let data = {}; | |||
|
|
|||
| if ( item.isComponent && item.children?.length ) { | |||
| if ( item.type === 'component' && item.children?.length ) { | |||
There was a problem hiding this comment.
I didn't see this change documented in the changelog, but without it, the badges won't appear - isComponent doesn't exist, but I saw type is component, so we're using that.
| @@ -2,11 +2,13 @@ | |||
| * External dependencies | |||
| */ | |||
| import styled from '@emotion/styled'; | |||
| import { Icons } from '@storybook/components'; | |||
There was a problem hiding this comment.
Icons from @storybook/components is deprecated in favor of @storybook/icons, see https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#icons-is-deprecated
| @@ -28,7 +29,7 @@ The site shows the individual components in the sidebar and the Canvas on the ri | |||
|
|
|||
| To view the documentation for each component use the **Docs** menu item in the top toolbar. | |||
|
|
|||
| To view the source code for the component and its stories on GitHub, click the <InlineIcon icon="repository" /> View Source Repository button in the top right corner. | |||
| To view the source code for the component and its stories on GitHub, click the <InlineIcon icon={ RepoIcon } /> View Source Repository button in the top right corner. | |||
There was a problem hiding this comment.
We're using RepoIcon from @storybook/icons directly since Icon from @storybook/components is deprecated, see https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#icons-is-deprecated
|
I think this is ready for review now! |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This was referenced Dec 6, 2024
This was referenced Dec 6, 2024
|
While testing this one, I've found a bunch of warnings and errors in our stories, and I took a small sprint to fix them:
|
tyxla
force-pushed
the
try/storybook-v8
branch
from
17ea62e to
7bfce92
Compare
ciampo
reviewed
Dec 11, 2024
There was a problem hiding this comment.
Apart from this comment, code changes seem good to me, although I'll spend some more time doing additional testing.
Thank you for looking @ciampo 🙇 Replied to your concerns, I think we still need to keep those spies (see my reply). Let me know if this is good to go once you're finished testing. |
ciampo
approved these changes
Dec 11, 2024
tyxla
force-pushed
the
try/storybook-v8
branch
from
7bfce92 to
aab89cd
Compare
Thanks for taking a look @ciampo! I've rebased to the latest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Upgrading Storybook to v8.0.
Why?
We've been stuck on Storybook 7.x for a while.
How?
Doing a gradual migration, one step at a time, migrating all necessary changes for 8.0.
Planning to incrementally get it to the latest version in subsequent PRs.
Use my self-review for more details on each change.
Testing Instructions
Testing Instructions for Keyboard
Same
Screenshots or screencast
None