-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 to cli #1561
base: main
Are you sure you want to change the base?
Conversation
@lloydrichards is attempting to deploy a commit to the shadcn-pro Team on Vercel. A member of the Team first needs to authorize it. |
I believe!!! |
@lloydrichards did you have any trouble with the code blocks showing corrupted code? Happens to me with |
I had a look and things loook correct in the code block for my dev env, but i've gone and upgraded storybook to v7.4.5 incase there was some bug in there somewhere. are you using |
i implemented a lot of stories myself before i found your repo. Now i cloned yours and am running it and got this error: just fyi. Seems like awesome work though. I've also built tests for about half of the components so far. i will have to write tests for all of them soon |
I think the issue is using npm vs pnpm. i usually use yarn and was running into issues with |
ahhh nevermind, i was on now it works |
@lloydrichards what do you think is the easiest way to add the import code block on each story at the top. like this: import { Button } from 'mylibrary/button'` Right now i'm having to make a .mdx for each story and put that at the top manually like this: |
The way its done through the CLI, is that within the /registry/ui components they reference using aliases: import { buttonVariants } from "@/registry/default/ui/button" and then later in the CLI process there is a pretty cool transform-import function ( This should also apply to the .stories.tsx files so if you use the |
f222e67
to
24727b8
Compare
@lloydrichards This looks great!!! 😁 is there anything that you need from me or the Storybook team to wrap up? |
One piece of the puzzle that is still missing is bring able to infer the arg types better for the components. Normally in a story the Meta does a pretty good job building out the controls, but i've noticed with the Radix components that this only works if you specify a prop in the Story.args. Do you know if there is a better way to infer the argTypes, otherwise its a manual process of picking which props are most useful for each story |
@shadcn Would it be possible to have a look and see if the changes im proposing here to the CLI and the inclusion o Storybook is inline with you roadmap with shadch/ui? So far I've achieved the core of what i want to do with this PR:
next steps for me really depend if there is any pushback from your side or any requests for changes. just let me know or if you don't have time, maybe ping another maintainer so I can try test the CLI? 🙏 |
This is most likely because Storybook, by default, filters out all props sourced from Try adjusting the prop filter like so: // .storybook/main.ts
import type { StorybookConfig } from "@storybook/nextjs"
const config: StorybookConfig = {
// ...
typescript: {
reactDocgen: 'react-docgen-typescript',
reactDocgenTypescriptOptions: {
shouldExtractLiteralValuesFromEnum: true,
// 👇 Adjusted prop filter
propFilter: (prop) =>
prop.parent ? !/node_modules\/(?!@radix-ui)/.test(prop.parent.fileName) : true,
},
},
}
export default config |
Signed-off-by: Xavier Geerinck <[email protected]>
There is still am issue with <Story /> in .mdx, will be easier to refactor into .stories.tsx
Any help required at |
I think whats needed at this point is for @shadcn to review and give any feedback on changes or additional requirements. The PR is still part of the |
@lloydrichards I noticed in your storybook some of the components are corrupted: I am having this same problem in my other implementation of this. it is not corrupted locally, only on the deployed instance. |
This would be a good question for @kylegach and the storybook team, I've ran into similar issues with using the children as an arg prop (guessing its something to do with stringifying JSX components). The other day I ran into a bug where if i wanted to render a ReactNode inside braces, using a template literal creates: const stringComponent: ReactNode = foo(); // <-- "hello"
...
return ( // <-- "([object Object])"
<p>
{'(${foo()})'}
</p>
) but using string concatenation rendered correctly: return ( // <-- "(hello)"
<p>
{"("}
{foo()}
{")"}
</p>
) |
I figured out how to fix my implementation. I upgraded to latest v8, and also had to change all of my displayName to be raw text: |
Took me a little while to find time to add the stories for the New Chart components, but they're done now. Didn't want to go too complicated so tried to use the same dataset and config for as many chart types as was appropriate. |
This usually happens if you are using a component that is wrapped with a memo, forwardRef, etc. and not using a displayName... Yae, I experienced the same issue in the past. |
@shadcn any update when we can see this coming? :) |
@shadcn Looking forward to your approvement |
If anyone in 2024 is here looking for answers about pulling these components into a SB, this was the answer for me, with some revisions: // in .storybook/main.ts config
typescript: {
reactDocgen: 'react-docgen-typescript',
reactDocgenTypescriptOptions: {
shouldExtractLiteralValuesFromEnum: true,
shouldExtractValuesFromUnion: true,
shouldIncludePropTagMap: true,
shouldRemoveUndefinedFromOptional: true,
// 👇 Adjusted prop filter
propFilter: (prop) =>
prop.parent
? !/node_modules\/(?!@radix-ui)/.test(prop.parent.fileName)
: true
}
}, |
@shadcn I've refactored the registry changes I made previously to match the new 2.0 CLI (which has made it much easier to extend. 😘👌) mostly through extending the registry item type with Have tested building the registry but not included the built files as it makes reviewing the PR quite overwhelming, but feel free to open the changes and test out the output yourself (and before committing the new output for testing I assume). Am still motivated to get this PR merged so let me know if there is anything you need from my end or if you have some changes you would like me to tackle 🙏 |
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.
Great effort!
Would love to see this changed merged, as adding the storybooks by hand takes up some valuable time.
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.
support for more deep object in colors
colors: {
primary: {
someKey: {
DEFAULT: "***",
...
500: "***"
}
...
}
...
}
.map(([name, colors]) => { | ||
return { | ||
name, | ||
colors: typeof colors === "string" ? { [name]: colors } : colors, |
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 does not cover when user update the config to be more have more object in their color systems.
Maybe we can do flatten the object here instead?
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.
https://github.com/hughsk/flat/blob/master/index.js
can use util function from "flat" to flatten the object.
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.
colors: typeof colors === "string" ? { [name]: colors } : colors, | |
colors: typeof colors === "string" ? { [name]: colors } : flatten(colors), |
drawer meta has both ´satisfies Meta<typeof Drawer>´ and ´:Meta<typeof Drawer>´
Goals/Scope
This is extension of PR #11 regarding adding stories for storybook through the CLI. A lot of work was already done by XavierGeerinck to get this going, but the PR seems to have been lost despite renewed interest and push to get this feature added. I've created this renewed PR with the hope of getting some attention and momentum in adding this to the CLI.
For the sake of narrowing the scope of this PR, i've removed the typography component so we can focus on just storybook.
Description
There are two main areas that have been changed in this PR, first is the addition of a storybook folder in the /registry which contain
*.story.tsx
files for most of the component (some are still missing). Second is I've added an additional option to thecomponent.tsx
which should allow for the stories to be loaded in through the CLI when adding a component (still need to test)Component Stories
The stories are added to the
registry
folder in the*.story.tsx
files. While most of the components follow the examples in the shadcn/ui docs, some of the examples needed to be simplified or broken up into multiple stories to make them more readable. Below you will find a list of the components and their stories.Expand Component Table
Comments
I'm very happy for support with getting this PR through so if anyone would like to review the stories, or add new ones, or help with upgrading the CLI, please join the conversation in the comments 🙇
if you would like to add a missing story, you can create the
.story.tsx
file in the/apps/www/registry/stories/
directory and add the file to theapps/www/public/registry/index.json
for the correct component.