-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: add Storybook starter #2381
Conversation
Run & review this pull request in StackBlitz Codeflow. |
@manucorporat The static storybook build doesn't work with Qwik, so we're unable to deploy preview versions. Is this something you could help with? |
yes! i would love to help, are you in discord? |
Yes! You can find me under DustinJSilk#5412, just sent you a message as well |
One more issue to note is that the
|
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'm just a random passerby who likes Storybook and Qwik, so take my comments for what they're worth. It's exciting to see this coming!
Maybe it would be a good idea to merge this with the custom configuration now, and come back with simplifications later if we can get a storybook framework working.
(I'm slightly interested in helping make a sb framework if needed, but I don't want to promise anything because I can't guarantee I'll have time to do it)
packages/docs/src/routes/qwikcity/integrations/storybook/index.mdx
Outdated
Show resolved
Hide resolved
I did a little more digging around and it seems like this configuration wont work with the next versions of storybook + builder-vite. @literalpie I've taken your advice and I'll get a PR sent to the Storybook repository ASAP |
Actually scratch that, it works quite well with the |
Fixed storybook dependencies to Final issue remaining is HMR - or finding a way to refresh the iframe when a component updates |
I made a I think having a framework package will be beneficial so improvements can be made to the configuration without consumers needing to change anything. If you continue with this PR as is, I can make a follow-up to utilize the framework package (or you can use it now if you want) |
@literalpie, yes that would be nice, have you tried sending a PR to storybook to get it merged in and officially supported there? |
@DustinJSilk Based on this comment, I think they prefer to let framework packages stabilize before they pull them into the monorepo. I'm guessing at a minimum, we should add better documentation, types, and follow the steps in documentation from that PR more closely (I'll try to circle back soon and do that soon) |
Hi, an update on progress with this: I've made a few new releases of my framework (0.0.3 is the latest right now). It now refreshes stories when code changes are made. Otherwise, it basically behaves the same as the setup in this starter. I posted in the Storybook Discord, and they pointed me in a direction I can go to try getting I think my ideal setup would be that |
9ad387e
to
feba151
Compare
@literalpie I've updated this PR to include your library, well done! |
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.
Looks good. One possible suggestions
|
||
const config: StorybookConfig = { | ||
addons: ['@storybook/addon-essentials'], | ||
stories: ['../src/**/*.stories.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'], |
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.
The storybook cli scaffolds *.mdx
for all frameworks (not *.stories.mdx
). I'm guessing this is because stories.mdx
is a special pattern that will make the file declare stories rather than just be documentation.
Picking up all mdx files can cause conflicts with Qwik-city mdx though, so having some explicit indicator that mdx is for storybook, not qwik city is important (because Qwik-city mdx will only work with Qwik component, and SB mdx will only work with react components).
I think a "storybook" prefix gets the explicit marking, without triggering the special behavior of stories.mdx
.
stories: ['../src/**/*.stories.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'], | |
stories: ['../src/**/*.storybook.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'], |
But, because of some limitations, I'm planning on just doing *.mdx
in the storybook CLI qwik integration like other frameworks. I'll need to add documentation to warn about the Qwik-city conflict potential. You could do that if you want to match how the sb cli will eventually work (based on my current guess).
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.
Using stories.mdx
is the recommended approach to defining stories in mdx files. So this should keep it consistent with what storybook users expect and avoids adding Qwik mdx files as stories. There is of course still the issue of any mdx files in the routes folder being turned into a page but I think thats fine for now as most stories will be on a component level.
What is the special behaviour from a .stories.mdx
file that we're trying to avoid by using .storybook.mdx
?
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.
in a stories.mdx
file, any story you reference will get added to the navigation. If you already declared the story in a stories.tsx
file, this will cause duplicates.
with any other pattern, you can reference the stories in the mdx without adding them to the side bar.
I suppose it could go either way, but I defer to the default in storybook CLI
Qwik support has been added to the storybook CLI. You can now add storybook to a qwik project with |
Closing this PR since the documentation & configuration exists in the storybook repository and in storybook-framework-qwik. |
What is it?
Description
Adds a new starter script to add Storybook to a project.
Use cases and why
Checklist: