-
-
Notifications
You must be signed in to change notification settings - Fork 38.1k
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(tools): create ui-components package and setup Storybook #41920
feat(tools): create ui-components package and setup Storybook #41920
Conversation
client/.storybook/manager.js
Outdated
@@ -0,0 +1,6 @@ | |||
import { addons } from '@storybook/addons'; |
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 file wasn't generated by the script. I created it as part of the custom theming.
Ref: https://storybook.js.org/docs/react/configure/theming#create-a-theme-quickstart
client/.storybook/theme.js
Outdated
@@ -0,0 +1,8 @@ | |||
import { create } from '@storybook/theming'; |
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 file wasn't generated by the script. I created it as part of the custom theming.
Ref: https://storybook.js.org/docs/react/configure/theming#create-a-theme-quickstart
client/.storybook/theme.js
Outdated
brandImage: | ||
'https://cdn.freecodecamp.org/platform/universal/fcc_secondary.svg' | ||
}); |
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 browsed the cdn repo but couldn't find a logo file that has both text and gray90 background color. I resorted to the secondary logo as the text of the primary one is not readable on Storybook's default background.
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 have png assets with set backgrounds on the design style guide. https://github.com/freeCodeCamp/design-style-guide/blob/master/downloads/fcc_primary_large.png but they are not on the CDN.
We could default to the gray85 for the background and use the primary SVG down the road.
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 just tried it out and the PNG doesn't seem to have a background either
Storybook has very limited theming options. In order to have a dark background for the primary logo, I would need to set gray85
to the entire sidebar and change the text color; but it affects the Controls panel, which doesn't have dedicated styling options.
So I think we would still need an image file with a set background on CDN if we want to use the primary logo.
client/package.json
Outdated
"storybook": "start-storybook -p 6006", | ||
"build-storybook": "build-storybook" |
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.
These were added by the Storybook script.
client/package.json
Outdated
"@babel/core": "^7.13.16", | ||
"@storybook/addon-actions": "^6.2.9", | ||
"@storybook/addon-essentials": "^6.2.9", | ||
"@storybook/addon-links": "^6.2.9", | ||
"@storybook/addon-postcss": "^2.0.0", | ||
"@storybook/react": "^6.2.9", | ||
"@testing-library/jest-dom": "5.12.0", | ||
"@testing-library/react": "11.2.6", | ||
"autoprefixer": "9.8.6", | ||
"babel-loader": "^8.2.2", |
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.
All of the new dependencies were added automatically by Storybook, except for @storybook/addon-postcss
.
client/package.json
Outdated
"dotenv-webpack": "^7.0.2", | ||
"html-webpack-plugin": "^5.3.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.
If I understand storybookjs/storybook#14403 correctly, Storybook is using a very old version of these libraries. They are hoisted and get picked up by Webpack, but error thrown because they are not compatible with Webpack 5.
Apparently Storybook team fixed it but then reverted the PR due to it being backward incompatible (storybookjs/storybook#14403 (comment)). I followed storybookjs/storybook#14497 (comment) and added these packages to our dev dependency list as a workaround.
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 team recommends using Yarn's resolutions
to workaround this issue. It is not applicable to us, but I'm just posting the link here for future reference: https://gist.github.com/shilman/8856ea1786dcd247139b47b270912324#yarn-resolutions
@@ -0,0 +1,211 @@ | |||
import { Meta } from '@storybook/addon-docs/blocks'; | |||
import Code from './assets/code-brackets.svg'; |
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 file is generated by Storybook, along with a bunch of svg files. It has links to the official document pages, which I think might come in handy especially when we don't have documentation on our side yet.
I decided to keep this one, and we can delete it once our component library is more mature.
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.
Nice work @huyenltnguyen!
I just wanted to drop in and give some quick feedback about the structure. I think the best place to develop and design the components would be within the tools
location. We can add additional directory within the tools called ui-components
and name the package @freeCodeCamp/ui
- not sure we will publish it though.
I say this because we may want to extend the components well beyond their use in the client app (which is architecturally speaking a consumer).
The ability to build components in isolation would force to think of them as generic components vs customizing too much.
IMHO, if we use tailwind with headless-ui I am convinced 90% of our needs will be addressed already.
With storybook we can address the custom needs like layouts, nav and theming.
Thinking ahead when we have additional apps (like a JAMStack news) this could be the common locations to consume components from.
P.S: This will also keep the client app lean.
@raisedadead Totally agreed! I'm also in favor of keeping the components platform-agnostic, so it makes sense to move it out of I'll try to get back to this PR over the weekend 😄 In the meantime, I'm wondering if you have any suggestions on the folder structure. Storybook's default structure is quite flat:
I'm thinking about having a folder for each component, which would be consistent with our
One thing I completely forgot in the issue discussion is that we should write unit tests for our components as well. For components that I have some experience working with Jest and @testing-library/react and I guess I'm going to propose that 😛 But I would like to get some input from the core team on the testing approach as well. |
I like the idea of having a simple flat structure instead of layers like we currently do. One thing to note is that all file names should be kebab-case As for tests, YESSSS PLEASE!!! we are fans of RTL so that should already align with what we want. Please go ahead. |
This reverts commit f55e249.
191afcb
to
c62919c
Compare
c62919c
to
30570be
Compare
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, the PR is ready for another review round.
I reverted the changes to start clean, then:
- cd into
tools
and rannpx sb init --type react
. The command is from the Troubleshooting section in the Storybook installation guide - The
package.json
file generated from the script didn't not include React. I believe this is because Storybook is meant to piggyback a project with a framework installed. I addedreact
andreact-dom
to the dependencies list to allow Storybook to run - The rest are the same as my initial setup
What I think I'm missing here is the setup for package publication. I will need to read up on this as I have never released a package before.
I will leave Tailwind and RTL setup for another PR. They are already in the TODO list #41580 (comment) 🙂
I also like having a simple structure for the components that share the same hierarchy. Maybe we should consider adding another directory when the components are not atomic and are a combination of UI elements such as forms, navigation, etc. |
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.
Going to go ahead merge, and unblock you from spiking further.
Checklist:
Update index.md
)main
branch of freeCodeCamp.Description
Ref #41580
This PR sets up Storybook in the
client
folder. I try to keep the PR small with some basic configs, and will have follow-up PRs to iterate. Storybook right now has a stub component that was generated by Storybook's installation script.To start it, run:
I will add comments to the code to give a better explanation, but here is a breakdown of what I did:
npx sb init
inside/client
. The script creates a.storybook
folder right under/client
, and astories
folder under/client/src
Button
component only and converted it to JavaScript. The component list that the script generates can be found here, and the generated introduction file can be found here@storybook/addon-postcss
as a dev dependency and added it to the.storybook/main.js
configdotenv-webpack
andhtml-webpack-plugin
as dev dependencies as a workaround for Webpack5:start-storybook
fails when.env
is present storybookjs/storybook#14403 (I will elaborate more in the comment)package.json
fileScreenshot