ethereum / ethereum-org-website

Ethereum.org is a primary online resource for the Ethereum community.
https://ethereum.org/
MIT License
5.05k stars 4.8k forks source link

RFC: Implementing StorybookJS & Chromatic #8490

Closed TylerAPfledderer closed 1 year ago

TylerAPfledderer commented 1 year ago

Overview

This is a proposal to add StorybookJS to the workflow of the Ethereum.org site, outlining the potential benefits to its implementation beginning Q1 of next year after the Chakra UI migration is complete and serve as a parallel epic to the Design System Epic (#6284). Anyone is welcome to comment in favor of or against what is suggested here.

There will also be an upcoming PR to demo the initial setup of Storybook and use with a couple of the existing components.

I will also include a possible alternative at the end.

Implementation timeframe and demo PR suggested by @pettinarip

Purpose

The idea of implementing StorybookJS comes from two current activities in the sites renovation:

  1. Migration to a new UI (Chakra UI)
  2. Creation of an open design system

With the site being large-scale in content with design updates and future changes currently underway, the interest in testing components visually in isolation and checking for visual regression might be greater than past discussions on its use. During the migration to Chakra UI, there have been GH issues appearing related to regressions, in addition to struggles in easily checking for changes to components on pages in the local environment (environment setups might still vary, and this my personal take).

It is suggested to wait until Q1 so that it is not apart of the current migration roadmap and become a forced additional step in order to cross the finish line with PRs submitted by the community. This can also serve as the basis for the new design system where new components are expected to be made along with modifying existing ones.

Part of this proposal is to also include Chromatic to the workflow as the means to check and view visual regressions and changes for approval prior to PR merges.

In addition to visual testing, you can do interaction (integrating Jest + Testing Library), accessibility, and snapshot testing within Storybook.

Storybook also has a Showcase page to see it's implementation with existing design systems for companies like BBC, Twilio, and Github

Steps in the setup

These steps will be applied in the demo PR to be created

Alternatives

There are certainly many alternatives to StorybookJS. One I am personally aware of that is open-sourced is Ladle that is built specifically for React, and the visual regression tool Lost Pixel which can be used for Ladle (Chromatic can only be used with Storybook).

However, I can not comment on this or other options, as I have only used Storybook. The argument for a tool like Ladle is that it is a smaller package that uses the Component Story Format like Storybook but with a small dependency load, making it much faster than Storybook. However, it is not as mature as Storybook with arguably not as much support or features.

Open to comments on such alternatives. I provided Storybook as the proposal as it is widely used and therefore probably the most accessible for anyone contributing to the project.

TylerAPfledderer commented 1 year ago

Has there been discussion regarding adding variants to components in the theme config instead of passing props to them at the page level?

This could help in testing different contexts in Storybook without creating page-level stories just for this.

Please reference here any existing public discussions about this. 😄

samajammin commented 1 year ago

Hey @TylerAPfledderer - thanks for this suggestion!

I've used Storybook on projects in the past & have enjoyed it. Agree it can help to avoid regressions/bugs when making updates to components! It also makes components easier to test & interact with during development. I'd say the primary downside is it adds additional overhead to the project in terms of complexity & maintenance, similar to a test suite. But given that we don't have any test suite in the project 😄 I think Storybook could act as a solid check here!

When we initially considered (& discarded) the idea to add Storybook, the project was also at a different stage. The site was much smaller, we had less components overall & we had a small core team doing most of the development. The overall value of Storybook was smaller given we could quickly coordinate internally & easily test out components on pages in our dev environment.

Now that we've grown in size, both from the size of the codebase & number of contributors, I'm definitely open to the idea of rolling out Storybook across the project. I think it could pair well with the implementation of our new design system (#6284).

It is suggested to wait until Q1 so that it is not apart of the current migration roadmap and become a forced additional step in order to cross the finish line with PRs submitted by the community.

I agree with this. I think we should wait until #6374 is complete, so that we're not creating additional dependencies on that rollout. My initial suggestion on sequence would be:

  1. Finish the Chakra migration across components (#6374)
  2. Complete designs of the design system (#6284)
  3. Add Storybook to the project (#8490)
  4. Roll out implementations of the design system (i.e. update existing components & build new components to match designs). As part of this work, we could make it a requirement to create a Storybook component as part of the style updates.

Thoughts?

Part of this proposal is to also include Chromatic to the workflow as the means to check and view visual regressions and changes for approval prior to PR merges.

I'm not very familiar with Chromatic - looks neat. Do you see this as something that must be completed in tandem with adding Storybook? Or could this part be decoupled & potentially added afterwards? First gut reaction is to reduce complexity as much as possible here but if this would be an easy add to have a live instance of Storybook, then I'd be open to considering it.

I welcome additional input on this as well as how we think we'd want to structure Storybook if we do want to move forward with it.

TylerAPfledderer commented 1 year ago

@samajammin thank you for your thoughts! It makes sense to me the concerns that were present with the overheard.

If it helps at all, I find that the tutorial on Design Systems with Storybook is great to view to see what Storybook generally recommends in setting up the workflow for design systems.

I think Chromatic is an easy addon, but it can certainly be implemented after, since you can still publish the existing stories to it's cloud for a baseline for future changes, along with implementing the github actions to be able to review changes and and approve/deny the changes through the dashboard. If a change is denied, then it will fail the UI test, and then fail the PR to prevent merging.

Take look at the tutorial step Review with Teams for overview and basic setup.

  1. Finish the Chakra migration across components (Epic: Implement UI library #6374)
  2. Complete designs of the design system (Epic: open design system #6284)
  3. Add Storybook to the project (RFC: Implementing StorybookJS #8490)
  4. Roll out implementations of the design system (i.e. update existing components & build new components to match designs). As part of this work, we could make it a requirement to create a Storybook component as part of the style updates.

Based on your suggestion in the rollout, Chromatic can be the first step in No. 4 so we can track visual changes when implementing the design system.

For Chromatic setup:

  1. Setup an account on Chromatic (setup for an organization mirrors Github)
  2. Run command to deploy projects storybook to Chromatic
  3. Create github action to run Chromatic during PRs
  4. When creating a PR, a new storybook snapshot is built from the Github action for reviewers to view, and a separate action for UI testing to visually see the UI diffs. See tutorial section Test to Maintain Quality
  5. If a change is denied, the check in the PR fails to prevent merging.
TylerAPfledderer commented 1 year ago

Noting here of v7 of Storybook, still in beta at the time of this comment.

This can be explored as an upgrade in the future, should it be decided to bring Storybook online prior to v7 exiting beta.

pettinarip commented 1 year ago

@TylerAPfledderer FYI I've created a CHROMATIC_PROJECT_TOKEN secret on GH to let you create the proper jobs to do the Chromatic integration.

Let me know if this unblocks you from moving forward with Chromatic or if we prefer to use a different testing/personal account (to troubleshoot potential problems faster) before we use the "prod" account.

===== As I said on the Storybook PR, I think we can merge that PR and start creating stories for components as we implement the DS. Would be great if we could also have Chromatic working in parallel as we do the implementation.

TylerAPfledderer commented 1 year ago

@pettinarip should this now be considered closed?

pettinarip commented 1 year ago

Yes! great work @TylerAPfledderer 💪🏼