-
Notifications
You must be signed in to change notification settings - Fork 18
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
[T] Update Storybook to latest version "6.0.12" #512
[T] Update Storybook to latest version "6.0.12" #512
Conversation
Storybooks uses Component Story Format (CSF), if we export something that don't follow this format, the application crashes. I extracted the function into a file to prevent this error.
@@ -75,8 +77,8 @@ | |||
"node-fetch": "^2.6.0", | |||
"node-sass": "4.13.1", | |||
"raw-loader": "3.1.0", | |||
"react-is": "^16.13.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.
Is there a particular reason to add this dependency?
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.
This dependency is used to write MDX files.
https://storybook.js.org/docs/vue/writing-docs/mdx
If we don't intent to write MDX we can remove it =D
What do you think?
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.
Ok! That feature looks pretty cool!
Maybe we could evaluate in a couple of months if we're using it or not?
If we are not using it, then remove it?
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.
Sure, it works for me =D
src/stories/HdButton.stories.js
Outdated
@@ -1,96 +1,198 @@ | |||
/* eslint-disable import/no-extraneous-dependencies */ |
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.
Is this comment still needed on all files?
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 pointing out, we can't remove for the files that are still importing dev dependencies.
But for sure, I've forgot to update the Button story and to remove the comment in some files =D
Here are the changes:
/* eslint-disable import/no-extraneous-dependencies */ | ||
import { storiesOf } from '@storybook/vue'; | ||
/* import { storiesOf } from '@storybook/vue'; |
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.
This is a work in progress, right?
Make sure to remove these comments when you're finished hehe...
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! I've forgot to remove them >_<
Here is the change:
755364d
Default.parameters = { | ||
docs: { | ||
description: { | ||
component: HdResponsiveNote, |
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.
Cool!
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.
It looks great! I added a comment to remove some commented lines.
Would you create a new ticket to rewrite the remaining stories to the new format?
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 tested it and it seems fine. Reaaaaally appreciate the huge effort! I've dropped a comment regarding the HdButton
stories.
I think you will need to update the Hygen scaffolding template for stories, right?
Another thing, have you tested Percy
? Does it still work? Because we had the skip
parameter for HdAlert
, for example, and it is not there anymore. --> We can disable (skip) it for all stories for now and get back to it after, otherwise it might become a hell of a PR to review.
:disabled=disabled | ||
@click="onClick" | ||
>{{ text }}</HdButton> | ||
<div v-if="isInDarkBackground" :style="styleDarkBackground" /> |
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 think we lost this guy in here which used to trick the dark background. @metal-gogo I think you can input here as well.
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.
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.
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.
OMG, I didn't know about this background option. Can we make it enable it as soon as we select the "isInDarkBackground"? 😄
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've tried it out, unfortunately we don't have access to addon methods during a Story render.
Maybe I'm wrong, but I didn't find out on how to do it 😅
Yes, this was a suggestions from @ilyasmez We can calmly migrate the format, we have time until the version 7.0 😆 |
Thanks man =D, and answering your questions:
If you think it's nice to maintain the Playground, I can definitely put it back.
|
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.
What an achievement! 🎉 🎉 Generally, I like all features but especially ArgTypes
feature is poetical. Stories are looking better now.
Also, thanks for your detailed explanation, I suggest writing a Medium Blog about it!
@kuroski the Percy thing was just a ping, I didn't test it myself. I don't think we need the playground anymore. This new version of Storybook tackles things differently which is nice =] Nevertheless, removing from CI/Token should be enough. If it is not causing any issues, just keep it the way it is. |
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.
🚀
This PR closes ticket FET-198
In this PR:
notes add-on
are updated to use theStorybook docs
Changes and Deprecations
You can check deprecations and changes in this page
Here are some relevant points:
storiesOf
method from@storybook/vue
is still supported, but now Storybook recommends writing the files using Component Story Format (CSF)Knobs
are still supported, but we should avoid use them in prol of Controlsaddon-notes
is now in deprecation stage, we must use docs pagevue-cli-plugin-storybook
to use the default configuration that comes originally with storybookMigrating to Component Story Format (CSF)
Let's use the
HdBadge
component as an exampleBefore
After
And here is the result:
API
You can read in details through Storybook documentation, but I will highlight the most important concepts bellow.
Component Story Format (CSF)
Default export
The default export defines metadata about your component, including the component itself, its title (where it will show up in the navigation UI story hierarchy), decorators, and parameters.
The component field is optional (but encouraged!), and is used by addons for automatic prop table generation and display of other component metadata. title should be unique, i.e. not re-used across files.
Named story exports
With CSF, every named export in the file represents a story function by default.
We should follow this pattern because its a limitation for the Vue side.
Since the component props are automatically injected, we must pass through them to the named export component, otherwise the application will crash.
The name of the export will be converted into the story title, e.g:
Will be converted to
My Basic Component
Also, the exported component can override the default configurations
Args story inputs
Starting in SB 6.0, stories accept named inputs called Args. Args are dynamic data that are provided (and possibly updated by) Storybook and its addons.
ArgTypes
ArgTypes are a first-class feature in Storybook for specifying the behaviour of Args. By specifying the type of an arg you constrain the values that it can take and can also provide information about args that are not explicitly set (i.e. not required).
You can also use argTypes to “annotate” args with information that is used by addons that make use of those args, for instance to instruct the controls addons to render a color choose for a string-valued arg.
Arg types are automatically generated, but you can customize any property of the component, in the end, we will have a Args table in the documentation
Storybook uses behind the scenes vue-docgen-api to infer the props and events from the component.
The controls that will appear in the Args table can be changed to any of the options that you can find here
Result
You guys can check a DEMO here:
https://blocks-storybook-6.netlify.app/
PS
The commit 42a5cef can be reverted once we convert the component to the CSF format, since we can do this