-
Notifications
You must be signed in to change notification settings - Fork 8
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 upgrade #1134
Merged
Merged
Storybook 7 upgrade #1134
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
jattasNI
commented
Mar 24, 2023
fredvisser
reviewed
Mar 29, 2023
fredvisser
reviewed
Mar 29, 2023
change/@ni-nimble-components-1fb65473-c803-4f85-ad46-02ad52a5b22b.json
Outdated
Show resolved
Hide resolved
fredvisser
reviewed
Mar 29, 2023
packages/nimble-components/src/anchor/tests/anchor-matrix.stories.ts
Outdated
Show resolved
Hide resolved
fredvisser
reviewed
Mar 29, 2023
packages/nimble-components/src/theme-provider/tests/color.stories.ts
Outdated
Show resolved
Hide resolved
rajsite
approved these changes
Mar 30, 2023
rajsite
reviewed
Mar 30, 2023
5 tasks
1 task
1 task
rajsite
added a commit
that referenced
this pull request
Apr 3, 2023
# Pull Request ## 🤨 Rationale ## 👩💻 Implementation As part of #1134 the menu element definition added an import to the anchored region as an unused variable for the element registration side-effect. Build tooling with dead code elimination may try to strip out that dependency under the assumption that imports do not have side effects so they can safely remove an unused variable and the associated import tree. Switched to the [side-effect only import syntax](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#import_a_module_for_its_side_effects_only) which is a stronger declaration to bundling tools that the import has side-effects. ## 🧪 Testing Relying on CI, no additional tests added. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
jattasNI
added a commit
that referenced
this pull request
Apr 4, 2023
# Pull Request ## 🤨 Rationale After #1134, the Actions tab in Storybook was no longer displaying events fired by components. This is one of the issues tracked in #1146. As described in the [Storybook 7 migration guide](https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#removed-auto-injection-of-storybookaddon-actions-decorator), the actions addon is no longer enabled by default and it must be enabled by configuring the story with `decorators: [withActions]`. However, the `withActions` decorator wasn't exported from Storybook in a way that we could import it (see storybookjs/storybook#21887). A Storybook dev commented on that issue with a workaround that we can use until they fix that bug. ## 👩💻 Implementation 1. Add a [TypeScript shorthand ambient module declaration](https://www.typescriptlang.org/docs/handbook/modules.html#shorthand-ambient-modules) for the import URL that contains `withActions`. See `/utilities/tests/storybook-declarations.d.ts`. 2. Update all stories that expose an action to import `withActions` and configure it as a decorator (also reordered a few imports to group them by package) ## 🧪 Testing Manually verified many of the stories that have actions are now firing them. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
rajsite
pushed a commit
that referenced
this pull request
Apr 12, 2023
# Pull Request ## 🤨 Rationale Right after I submitted the upgrade to a release candidate of Storybook 7 in #1134, I noticed they [released the official 7.0.0](https://github.com/storybookjs/storybook/releases). Adopting the latest patch addresses some of the issues in #1146. ## 👩💻 Implementation 1. Update to latest non-RC build in package.json and regenerate package-lock.json 2. Regenerate one patch file since the file it was patching has a new name 3. Add another patch file since they introduced another instance of storybookjs/storybook#21820 4. Move off deprecated API for specifying `sourceTransform` as described in [migration guide](https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#source-block) 5. Delete `storybook-declarations.ts` since this bug was closed storybookjs/storybook#21887 ## 🧪 Testing Explored Storybook. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
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.
Pull Request
🤨 Rationale
We want to be on the latest storybook version because it fixes issues we've had with dependencies and it enables more powerful docs pages via mdx and Component Story Format enhancements.
Notable changes:
👩💻 Implementation
Following the upgrade guide didn't work very well because it is buggy with a monorepo. Fred's general strategy was to run the upgrade command from within
nimble-components
then delete thenode_modules
andpackage-lock.json
that it creates there, then re-install everything from the root. The migration tool is responsible for the changes to:Story
->StoryFn
andstoryName
toname
withXD
decorator which is no longer supportedBeyond that there were a few other manual changes:
elementTag
exportnimble-menu
to explicitly depend onnimble-anchored-region
(see comment onmenu/index.ts
)storybook.ts
in response to storybook type changes🔜 Follow ups in future PRs
Fixing these issues is tracked by #1146 . I tried upgrading to ts 4.7 but it didn't fix any of them.
🧪 Testing
Chromatic caught several diffs which resulted in the manual changes described above 👍.
It's still showing new stories for the wafer map and button. (The wafer map one is caused by Chromatic getting confused when I tried renaming that story, ran a Chromatic build, and then reverted that change. I don't know the cause of the button change.)<- this went away on a subsequent buildAlso manually spot checked many of the stories across browsers, especially ones that have workarounds in them like table and theme-aware tokens.
✅ Checklist