-
Notifications
You must be signed in to change notification settings - Fork 225
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
Simorgh - Storybook Upgrade #9207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on the plugin fixes!
package.json
Outdated
"@storybook/addon-knobs": "6.2.9", | ||
"@storybook/addon-viewport": "6.3.1", | ||
"@storybook/react": "6.2.9", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be changed to 6.3.1
? When I checkout the branch and run yarn
and then yarn run storybook
, I get a notification in the terminal saying A new version (6.3.1) is available!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 🚀 I think they've just released 6.3.2 so may be worth bumping again, but I'm sure 6.3.1 is fine too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this upgrade 👍
So to make sure I understand we updating to the latest stable version of Storybook 6 to solve security problems?
I thought we were going to upgrade to a Storybook 7 rc to get around the emotion support problems too? Looks like that isn't finished though judging by this issue: storybookjs/storybook#13491
"web-vitals": "1.1.1", | ||
"ws": "7.4.6", | ||
"trim-newlines": "3.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these are the various pinned dependencies we had to avoid security issues that are no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :)
Storybook 6.3 was the RC version at the time we had that call - it is now stable. I don't think a 7.0 RC is available yet, unless I'm missing something - https://www.npmjs.com/package/@storybook/react |
This does seem to have caused 2 stories to stop working - the next PR is a rewrite of all stories, however, so will see if this is still an issue then. Confirmed the components still work, so the problem is restricted to storybook |
Part of #9198
Overall change:
Upgrade to latest version of Storybook.
Code changes:
simorgh/src/app/components/MediaPlayer/index.stories.jsx
Line 10 in 935c898
cross-team
label to this PR if it requires visibility across World Service teamsTesting:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive
)