-
Notifications
You must be signed in to change notification settings - Fork 3.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
Initial Storybook setup #110
Conversation
Storybook can be started using `npm run storybook`
import React from 'react'; | ||
import { SessionContext } from 'next-auth/react'; | ||
|
||
|
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.
style nit, can you delete this newline?
@@ -0,0 +1,26 @@ | |||
import React from 'react'; | |||
import { SessionContext } from 'next-auth/react'; |
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.
Style nit, can you order these alphabetically?
import { SessionContext } from 'next-auth/react'; | ||
|
||
|
||
import { Header } from '../components/Header'; |
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 it possible to import from src/components/Header
? If not, that's okay
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.
moved them into the same folder, think thats inline with #99
Great first step! I have a few small requested changes then I'll merge this in. |
Moved component files into their own directory and moved stories next to them. Fixed a bug with loading images
Deleted the empty lines :) I also moved the files so its more inline with future structure. |
would make sense the have the stories in another folder? i.e. not between all the components? maybe in I am suggesting this because stories are more of "write and forget", otherwise the files would be in the way all the time. @jojopirker @fozziethebeat what do you think? |
This is definitely a matter of preference and probably past experiences. I like how RedwoodJS put the test and stories in the same directory as the component so that when you make changes to the component you're reminded to check the story or test and don't have to go find the corresponding file in some other parallel directory structure. Let's try this for now and then re-evaluate. |
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 after a few tiny changes and running prettier.
website/src/pages/index.tsx
Outdated
@@ -7,7 +7,7 @@ import { CallToAction } from "src/components/CallToAction"; | |||
import { Faq } from "src/components/Faq"; | |||
import { Footer } from "src/components/Footer"; | |||
import { Header } from "src/components/Header"; | |||
import { Hero } from "src/components/Hero"; | |||
import { Hero } from "sr../components/Header/Header |
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 looks wrong, I think you meant to change line 9
title: 'Example/Header', | ||
component: Header, | ||
parameters: { | ||
// More on Story layout: https://storybook.js.org/docs/react/configure/story-layout |
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.
we can probably delete this in the code. If people get confused we can add explanations in a README.
import { Avatar } from "./Avatar"; | ||
import { Container } from "./Container"; | ||
import { NavLinks } from "./NavLinks"; | ||
import { Avatar } from "../Avatar/Avatar"; |
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.
Can these be absolute imports (src/components/Avatar/Avatar
)?
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.
Holding off on the approval until the merge fixes are in. I'd like to make sure those are all addressed and no new issues get introduced.
Should include all the fixes now. |
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!
Could you please also resolve the merge conflicts so we can merge this?
some imports did not work after merging
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.
while installing locally, I get the following in the console:
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @mdx-js/[email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react
npm WARN react@"18.2.0" from the root project
npm WARN 128 more (@chakra-ui/accordion, @chakra-ui/alert, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react@"^16.13.1 || ^17.0.0" from @mdx-js/[email protected]
npm WARN node_modules/@mdx-js/react
npm WARN @mdx-js/react@"^1.6.22" from @storybook/[email protected]
npm WARN node_modules/@storybook/addon-docs
npm WARN
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react
npm WARN peer react@"^16.13.1 || ^17.0.0" from @mdx-js/[email protected]
npm WARN node_modules/@mdx-js/react
npm WARN @mdx-js/react@"^1.6.22" from @storybook/[email protected]
npm WARN node_modules/@storybook/addon-docs
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react
npm WARN react@"18.2.0" from the root project
npm WARN 128 more (@chakra-ui/accordion, @chakra-ui/alert, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react@"^0.14.8 || ^15.0.1 || ^16.0.0 || ^17.0.1" from [email protected]
npm WARN node_modules/react-element-to-jsx-string
npm WARN react-element-to-jsx-string@"^14.3.4" from @storybook/[email protected]
npm WARN node_modules/@storybook/react
npm WARN
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react
npm WARN peer react@"^0.14.8 || ^15.0.1 || ^16.0.0 || ^17.0.1" from [email protected]
npm WARN node_modules/react-element-to-jsx-string
npm WARN react-element-to-jsx-string@"^14.3.4" from @storybook/[email protected]
npm WARN node_modules/@storybook/react
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react-dom
npm WARN react-dom@"18.2.0" from the root project
npm WARN 40 more (@chakra-ui/modal, @chakra-ui/portal, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react-dom@"^0.14.8 || ^15.0.1 || ^16.0.0 || ^17.0.1" from [email protected]
npm WARN node_modules/react-element-to-jsx-string
npm WARN react-element-to-jsx-string@"^14.3.4" from @storybook/[email protected]
npm WARN node_modules/@storybook/react
npm WARN
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react-dom
npm WARN peer react-dom@"^0.14.8 || ^15.0.1 || ^16.0.0 || ^17.0.1" from [email protected]
npm WARN node_modules/react-element-to-jsx-string
npm WARN react-element-to-jsx-string@"^14.3.4" from @storybook/[email protected]
npm WARN node_modules/@storybook/react
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react
npm WARN react@"18.2.0" from the root project
npm WARN 128 more (@chakra-ui/accordion, @chakra-ui/alert, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react@"^16.8.4 || ^17.0.0" from [email protected]
npm WARN node_modules/react-inspector
npm WARN react-inspector@"^5.1.0" from @storybook/[email protected]
npm WARN node_modules/@storybook/addon-actions
npm WARN
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/react
npm WARN peer react@"^16.8.4 || ^17.0.0" from [email protected]
npm WARN node_modules/react-inspector
npm WARN react-inspector@"^5.1.0" from @storybook/[email protected]
npm WARN node_modules/@storybook/addon-actions
npm WARN deprecated [email protected]: Chokidar 2 does not receive security updates since 2019. Upgrade to chokidar 3 with 15x fewer dependencies
Is this expected? storybook already has react 18 support, not sure why it is complaining here.
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 looks pretty good. Please make sure the merge with header.tsx works smoothly and nothing gets dropped.
merged the previous changes... i also moved all the files of the header into |
Thanks! I see a |
yup... i still need to learn how to properly merge with git^^ |
…2050) These `.jsx` components were added in #110, and they are the only ones that are still written in JavaScript in this project. Signed-off-by: Eng Zer Jun <[email protected]>
These changes enable to use
npm run storybook
to start storybook as mentioned in #102.There is an example story for the header.
Maybe should be moved to the components folder when doing #99.
What is not finished:
components/
rather thanpages/
What is finished: