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

chore: install Storybook addon themes #5785

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Conversation

HinataKah0
Copy link
Contributor

@HinataKah0 HinataKah0 commented Sep 12, 2023

Description

Install Storybook's addon @storybook/addon-themes and setup light & dark mode in Storybook

Followed this guide
See this comment for context

Validation

Tested locally using CrossLink component

export const Prev: Story = {
  args: {
    direction: 'left',
    text: 'How to install Node.js',
    url: 'https://nodejs.dev/en/learn/how-to-install-nodejs/',
  },
  decorators: [
    Story => (
      <div className="w-[305px]">
        <Story />
      </div>
    ),
  ],
};
Screenshot 2023-09-12 at 8 42 16 PM Screenshot 2023-09-12 at 8 42 33 PM

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

@vercel
Copy link

vercel bot commented Sep 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2023 0:28am

@github-actions
Copy link

github-actions bot commented Sep 12, 2023

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.24% (185/205) 75% (30/40) 86.66% (39/45)

Unit Test Report

Tests Skipped Failures Errors Time
14 0 💤 0 ❌ 0 🔥 3.535s ⏱️

@AugustinMauroy
Copy link
Member

should try not to use decorators in stories. And personally theme switch doesn't work on preview.

@HinataKah0
Copy link
Contributor Author

HinataKah0 commented Sep 12, 2023

About decorator usage

It is based on the collaborator guide

If you need to decorate/wrap your Component/Story with a Container/Provider, please use [Storybook Decorators](https://storybook.js.org/docs/react/writing-stories/decorators)

And personally theme switch doesn't work on preview.

Sorry, I don't get it. Which preview are you referring to?
Edit: Are you saying that theme switcher (the component that we will create later) won't work in Storybook preview?

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM! Clean implementation :)

@ovflowd ovflowd added the fast-track Fast Tracking PRs label Sep 12, 2023
@ovflowd
Copy link
Member

ovflowd commented Sep 12, 2023

I'd like to fast-track this as other PRs would definitely benefit from this trivial change. cc @nodejs/nodejs-website

@AugustinMauroy
Copy link
Member

I mean we should use theme switching on all stories.

@ovflowd
Copy link
Member

ovflowd commented Sep 12, 2023

I mean we should use theme switching on all stories.

This is already what this changeset does. Or so I believe

@HinataKah0
Copy link
Contributor Author

Yes, it changes all stories. You can switch between stories and the themes are the same.

@ovflowd
Copy link
Member

ovflowd commented Sep 12, 2023

@HinataKah0 can you run the CI? :)

@HinataKah0
Copy link
Contributor Author

I usually let some time passes before merging
@ovflowd If you think we can merge soon, I am fine with it

@ovflowd
Copy link
Member

ovflowd commented Sep 12, 2023

@HinataKah0 Im just asking for you to run the CI pipelines... not to merge the PR

@HinataKah0
Copy link
Contributor Author

My bad :)

@ovflowd ovflowd added this pull request to the merge queue Sep 13, 2023
Merged via the queue into nodejs:main with commit d411555 Sep 13, 2023
19 checks passed
@HinataKah0 HinataKah0 deleted the storybook-themes branch September 13, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track Fast Tracking PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants