-
-
Notifications
You must be signed in to change notification settings - Fork 680
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: storybook setup, chromatic setup and typography stories #3015
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3015--asyncapi-website.netlify.app/ |
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.
Do mention about the storybook setup and how to run storybook locally, in the README file.
package.json
Outdated
"storybook": "storybook dev -p 6006", | ||
"chromatic": "chromatic --exit-zero-on-changes", | ||
"build-storybook": "storybook build" | ||
}, |
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.
Make the script commands according to the pattern followed, like build:storybook
. Follow the same for all the scripts added.
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.
Done
package.json
Outdated
"eslintConfig": { | ||
"extends": [ | ||
"plugin:storybook/recommended" | ||
] | ||
} |
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.
Add the eslint config in the eslintrc
file only.
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.
Done
.github/workflows/chromatic.yml
Outdated
@@ -0,0 +1,24 @@ | |||
# .github/workflows/chromatic.yml |
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.
Have you tested this workflow in your forked repo? If yes, kindly share the workflow run link.
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.
Yes it's working
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 you provide the link when you executed in your forked repo?
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.
.github/workflows/chromatic.yml
Outdated
|
||
name: 'Chromatic' | ||
|
||
on: push |
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 it should be done at the PR level. So, you should run this workflow as per pull_request_target
.
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.
Okay Sure
.github/workflows/chromatic.yml
Outdated
with: | ||
node-version: 20 | ||
- name: Install dependencies | ||
run: npm ci |
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 installing the dependencies require npm i
command.
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.
Rectified this
.storybook/main.ts
Outdated
name: "@storybook/nextjs", | ||
options: {}, | ||
}, | ||
staticDirs: ["..\\public"], |
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.
Shouldn't we provide directory name as ../public
for all users?
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.
Rectified this
.storybook/preview-head.html
Outdated
<style> | ||
body { | ||
font-family: 'Work Sans', sans-serif; | ||
font-optical-sizing: auto; | ||
font-style: normal; | ||
} | ||
</style> |
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.
Better to have preview.css
file in this folder and import the CSS file 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.
Done
Rectified this. |
If you look into the console inside storybook preview, they aren't any Tailwind styles applied to it. Why it is not able to get Tailwind classes properties? |
@devilkiller-ag Can you please demonstrate this in Heading stories component, so that we are aware about it, how to resolve it. |
@devilkiller-ag Can you please demonstrate this in Heading stories component, so that we are aware about it, how to resolve it. Done. We will also have to disable Here is how it looks after update (Notice the change in description of all props in the table and the description of Heading page): |
@@ -107,8 +110,17 @@ | |||
"yaml": "^2.3.4" | |||
}, | |||
"devDependencies": { | |||
"@chromatic-com/storybook": "^1.5.0", |
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.
Remove chromatic library for now. We will look into it's use case after further discussions.
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.
Done
package.json
Outdated
"generate:tools": "node scripts/build-tools.js" | ||
"generate:tools": "node scripts/build-tools.js", | ||
"dev:storybook": "storybook dev -p 6006", | ||
"chromatic": "chromatic --exit-zero-on-changes", |
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.
"chromatic": "chromatic --exit-zero-on-changes", |
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.
Done
components/typography/Paragraph.tsx
Outdated
* @param {string} props.textColor contains text color for the paragraph. 'text-gray-700' is by default | ||
* @param {string} props.fontWeight contains class name for applying font weight in the paragraph | ||
* @param {string} props.className contains additional classes that should be added to the component |
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.
Add these comments as part of interface comments only.
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.
Done
chromatic.config.json
Outdated
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.
Remove this file.
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.
Done
|
||
type Story = StoryObj<typeof Paragraph>; | ||
|
||
export const Paragraphs: Story = { |
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.
Are we using these objects/functions anywhere else? If not, then why are we exporting 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.
No, we aren't using it manually. But storybook needs to have stories exported (Resource 1 | Resource 2). If we don't have stories exported they won't show up in the storybook.
@@ -0,0 +1,41 @@ | |||
import type { Meta, StoryObj } from '@storybook/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.
Why there are no warnings for ESLint in these files for adding Jsdoc comments?
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 didn't get this point, I haven't added any JSDoc in this file.
/rtm |
Description
This PR adds Storybook and Chromatic to the AsyncAPI Website for developing a UI kit for the website. Additionally, for testing the setup I have included the stories for the Typography (Headings, and Paragraph).
Here is the published storybook.