Skip to content
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

Implement StorybookJS #8497

Merged

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Nov 4, 2022

🚨 Note to Reviewers!

This is a PR for demo purposes only, and is not to be merged. It is related to RFC #8490 as a proposal to implement StorybookJS into the project.

This is to be closed once review, feedback, and all necessary discussion has been completed in determining whether or not to move forward with this implementation.

Originally a PR for demonstration, is now open to review and merge!

Primary reviewer requested is @pettinarip

Tied to #8490

Description

This is a demo showing basic implementation of StorybookJS with example stories to view some of the requirements in building stories for each component.

It is highly encouraged to request any specific use not already shown in the changes to further demonstrate potential benefits with this project, such as interactive parameters in the visual testing, tools to see responsiveness, actions, etc.

Notes on the Setup

The setup of storybook provided in .storybook includes additional configuration for its use in Gatsby (i.e. the webpack config) and the handling of controls with Chakra (i.e. the typescript option)

The Chakra addon is included additionally to set up the provider in the Storybook environment and include a simple dark mode toggle.

The @storybook/addon-a11y addon is another addition to the default to display the accessibility visual checks pertinent to each story,

Other addons can be explored to provide further usefulness.

Viewing the demo locally

  • After cloning this PR to your local environment, you will need to run yarn to update the installed dependencies.
  • Once dependencies have been installed, run yarn storybook to spin up the Storybook server.
  • Make note of how the spin-up takes in your environment. When spinning it up for the first time, it might take roughly 20 seconds in my local. Once it has cached, and you restart the server, it might boot quicker.
  • With this server open, the stories that were created should appear In the sidebar to select and view.
  • If you are new to Storybook, take a moment to explore all the features shown in the UI to see how they affect the stories. It is possible some might not worked due to how the example stories were created.

Chromatic

Chromatic is not added to this demo.

Resources

@github-actions github-actions bot added dependencies 📦 Changes related to project dependencies needs review 👀 Review is needed for this issue or pull request labels Nov 4, 2022
@gatsby-cloud
Copy link

gatsby-cloud bot commented Nov 4, 2022

✅ ethereum-org-website-dev deploy preview ready

@minimalsm minimalsm marked this pull request as draft November 4, 2022 15:56
@minimalsm
Copy link
Contributor

Thanks @TylerAPfledderer, converting this to draft for now :-)

@pettinarip pettinarip marked this pull request as ready for review November 14, 2022 12:56
@pettinarip pettinarip marked this pull request as draft November 14, 2022 12:57
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @TylerAPfledderer thanks! I've left a few comments there but overall this looks really nice.

I would want to understand better how we could share these stories with non-devs people (designers, etc). I don't know if you have some thoughts about this. For example, should we have a new endpoint in our dev env (https://ethereumorgwebsitedev01.gtsb.io/stories)? how do you see this aspect?

I like the component's folder structure, I think it makes more sense to have the .stories files next to the component rather than in a separate /stories folder.

Good job.

},
},
chakra: {
theme,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loved that we can test the color mode and different breakpoints from the chakra theme.

name: token,
styles: {
width: key,
height: "600px",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering why do we need this height 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a specified height set, an error would occur from the viewport tool:
Uncaught TypeError: styles.height is undefined

By the way, the viewport addon (part of the initial setup) comes with a collection of viewports that can also be applied to view stories with different common device specs.

"type-check": "tsc --noEmit"
"type-check": "tsc --noEmit",
"storybook": "start-storybook -p 6006",
"build-storybook": "build-storybook"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea of this build command to have a new endpoint where we can access all the stories all the time for each new build?

Thinking about our ci/cd pipeline, where do you see this applied?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he mostly addressed this here: #8497 (comment)

package.json Outdated
"@storybook/addon-essentials": "^6.5.13",
"@storybook/addon-interactions": "^6.5.13",
"@storybook/addon-links": "^6.5.13",
"@storybook/builder-webpack4": "^6.5.13",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webpack4 related packages shouldn't be necessary, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This was generated automatically on init. Weird that it did that along with the webpack5 installs.

Also, when reviewing with a fresh Gatsby project and checking out build processes when publishing storybook, webpack might be needed as a devDep due to a webpack module error

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Nov 15, 2022

I would want to understand better how we could share these stories with non-devs people (designers, etc). I don't know if you have some thoughts about this. For example, should we have a new endpoint in our dev env (https://ethereumorgwebsitedev01.gtsb.io/stories)? how do you see this aspect?

@pettinarip There are a couple of approaches to sharing storybook.

Because Netlify is being used for the site, you might decide to stick with that here and create the SB as a static app. In this case, a new site could be created pointing to the Ethereum repo, but when you configure the build the build command would be build-storybook and the published directory be storybook-static (this is the default directory name created)

Then make a more readable *.netlify.app domain name to use as the permalink to share on the site.

This would apply if you are looking to share it anywhere else.

Maybe you could assign storybook to it's own branch and deploy the branch so it can become a subdomain, but I have never tried that to see whether or not that would be a hassle. 😅

Alternatively, if Chromatic is set up, you can obtain a permalink from the dashboard to the published storybook and use that for sharing.

The Github action for Chromatic during PR provides a published Storybook to view with the changes. And within that Storybook, you can copy the URL for each changed component to also share that way.

Storybook provides an overview of Publishing statically and through Chromatic. But I have not found any solution to integrate it as an endpoint to an existing domain.

@samajammin
Copy link
Contributor

Amazing, thanks for pulling this together @TylerAPfledderer!

I like the component's folder structure, I think it makes more sense to have the .stories files next to the component rather than in a separate /stories folder.

I agree with you here @pettinarip, I think stories next to the component files (within the component directory) makes sense.

Alternatively, if Chromatic is set up, you can obtain a permalink from the dashboard to the published storybook and use that for sharing.

The Github action for Chromatic during PR provides a published Storybook to view with the changes. And within that Storybook, you can copy the URL for each changed component to also share that way.

From a brief look, Chromatic does like like a great solution for sharing Storybook preview deploys! I love the idea of viewing & approving the visual changes before fully approving PRs.

@samajammin
Copy link
Contributor

I'm curious what people think as an appropriate v1 for adopting Storybook? Which should we include?

  • Visual tests
  • Test runner
  • Accessibility tests (seems like the a11y add-on automatically runs these?)
  • Interactions tests
  • Snapshot tests

@samajammin
Copy link
Contributor

Also thinking in terms of a guide for creating stories, what should we enforce? e.g. should we expect stories for a component to cover all potential variations/props/states?

.storybook/preview.js Outdated Show resolved Hide resolved
.storybook/preview.js Outdated Show resolved Hide resolved
chakra: {
theme,
},
// Modify viewport selection to match Chakra breakpoints (or custom breakpoints)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet 👍

@samajammin
Copy link
Contributor

This is exciting 😄

@TylerAPfledderer what do you see as next steps? Anything we can do to unblock you? e.g. should we set up a Chromatic account & get you access so we could see what the integration would look like?

This is to be closed once review, feedback, and all necessary discussion has been completed in determining whether or not to move forward with this implementation.

Should this be eventually closed? Or might we want to merge this once we land on the approach we want to take?

@TylerAPfledderer
Copy link
Contributor Author

@samajammin I can certainly explore making a document outlining how to build the stories for the components in the project.

As to any current blocks, It would help if I could access a Chromatic project. Maybe first checking out the Access Control and Collaborators doc pages to make sure you can get an organization-level account set up and be able to give proper access to the project under the org.

I don't foresee any issue in run Chromatic in my local environment to initially build the library from a feature branch. (That really should matter?)

As to the question about whether to use this PR as the official setup, we can consider using this PR until there are any further mock changes or any outside changes in the project that might effect this where a fresh PR be more appropriate! 😁

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Nov 22, 2022

@samajammin I can certainly explore making a document outlining how to build the stories for the components in the project.

As to any current blocks, It would help if I could access a Chromatic project. Maybe first checking out the Access Control and Collaborators doc pages to make sure you can get an organization-level account set up and be able to give proper access to the project under the org.

I don't foresee any issue in run Chromatic in my local environment to initially build the library from a feature branch. (That really should matter?) At the very least, I can try to run Chromatic in my local feature branch with the token that is generated when setting up the project, so that there is a library to view on the dashboard. And I would be able to add a github action to the PR. Couldn't do much else.

As to the question about whether to use this PR as the official setup, we can consider using this PR until there are any further mock changes or any outside changes in the project that might effect this where a fresh PR be more appropriate! 😁

@TylerAPfledderer
Copy link
Contributor Author

Adding here a good overview blog as well from Storybook to give further ideas into integration.

https://storybook.js.org/blog/storybook-for-full-stack-developers/

@TylerAPfledderer
Copy link
Contributor Author

@samajammin and @pettinarip I have added an initial doc for adding storybook to a component.

It is certainly encouraged to provide questions, thoughts, and feedback to flesh this doc out further.

There are current thoughts and questions that come to mind.

In terms of fundamental creation of stories in conjunction to building the Chakra theme config for the new DS:

  • One story can render the set of variants of a component (if applicable)
  • One story can render the set of different sizes of a component (if applicable)
  • OR: the component may be primitive enough to show the above easily in a single story

It is very possible that many components will need just one story

When it comes to rendering content for components, where should they come from? (And possible keep from accessing the intl content directly)

  • Should there be a __mock__ folder that holds common content that could be provided to stories for multiple components?
  • Should there be the use of an addon like Mock Service Worker?
  • There can be the inclusion of an addon to address translations, such as Storybook i18n

Thank you for your continued support of this and I appreciate any thoughts to help in expanding this further. 😄

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loved the new doc! ❤️ amazing job overall in this PR @TylerAPfledderer

I liked and agreed on the file structure and stories creation you suggest here.

I think this is in a good shape to be merged as the first iteration of Storybook. Would encourage you to move it to Ready for review.

Once we merge this we can move forward with the Chromatic setup. As explained in the Chromatic docs, I'll add a CHROMATIC_PROJECT_TOKEN secret on GH. With that you should be able to create the GH job for chromatic and we could start testing that integration.

Let me know what do you think about this.


Each component will only need one file containing all the stories, and should follow the naming convention of the component.

So for the component `ExpandableCard.tsx`, the stories file will be names `ExpandableCard.stories.tsx`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
So for the component `ExpandableCard.tsx`, the stories file will be names `ExpandableCard.stories.tsx`.
So for the component `ExpandableCard.tsx`, the stories file will be named `ExpandableCard.stories.tsx`.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip All good with me! 🚀

Would it be alright if I remove the demo stories I provided and have this PR simply bring the Storybook setup to the project? It was not my intention to have them be apart of the project and would need to be replaced anyway.

@TylerAPfledderer TylerAPfledderer marked this pull request as ready for review January 11, 2023 03:27
@TylerAPfledderer TylerAPfledderer changed the title Demo StorybookJS Implementation Implement StorybookJS Jan 11, 2023
@pettinarip
Copy link
Member

@pettinarip All good with me! rocket

Would it be alright if I remove the demo stories I provided and have this PR simply bring the Storybook setup to the project? It was not my intention to have them be apart of the project and would need to be replaced anyway.

I think we can leave those stories to serve as quick examples. We can iterate and improve them along the process but it doesn't hurt us having them I'd say :)

I've triggered a new build on Gatsby Cloud.

Btw, there is a merge conflict. LMK if you need help on fixing that.

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Jan 11, 2023

@pettinarip Sounds good!

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip I'll need assistance with the conflict. I almost broke my local! 😅

@pettinarip
Copy link
Member

Ok @TylerAPfledderer I'll take a look at this later today.

@github-actions github-actions bot added the content 🖋️ This involves copy additions or edits label Jan 12, 2023
@pettinarip
Copy link
Member

Hmm for some reason, it is entering an infinite loop and the build is throwing a timeout error. Will look into it and try to see what is going on.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging it @TylerAPfledderer. Seems that there was a weird installation of babel. I did a reinstall and it worked fine.

Great first iteration on Storybook. Thanks for making this effort.

@pettinarip pettinarip merged commit de10ed9 into ethereum:dev Jan 13, 2023
@TylerAPfledderer TylerAPfledderer deleted the feat/demo-implement-storybook branch January 13, 2023 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits dependencies 📦 Changes related to project dependencies needs review 👀 Review is needed for this issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants