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

fix: storybook theme package not working #27086

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

garrettbear
Copy link
Contributor

@garrettbear garrettbear commented Sep 12, 2024

Description

This PR removes the storybook-dark-mode package and utilizes the native Storybook Theme API for managing the light and dark modes across both the documentation and component preview pages.

The motivation behind this change is to fix the bug introduced after upgrading Storybook to ^7.6.20 and storybook-dark-mode to ^4.0.2, which resulted in the following error:

Error: Storybook preview hooks can only be called inside decorators and story functions.

Additionally, we have updated the DocsContainer to ensure it handles the theme correctly without throwing errors. The bug prevented Storybook Docs pages from rendering properly.

Key changes:

  1. Removed the storybook-dark-mode package.
  2. Implemented the native Storybook Theme API to manage light/dark modes.
  3. Added a button to toggle between light, dark and side by side modes in the component preview area.
  4. Ensured the theme context is properly checked and applied, particularly in the Docs rendering.

Additional Context:

  • We will be implementing the shared UI Component library soon and we can revisit whether having the additional add-on (storybook-dark-mode) is valuable enough to add an additional dependency. We can add it to our roadmap for discussion when we are working on the storybook portion of our roadmap.

Possible Asked Questions

  • Did you consider that the app theme is not changing?
    Yes. It is set to the users system, so if the user uses dark mode then the application will run in dark. Made attempts for a fix
  • Did you consider a fix with the existing package?
    Yes. The best solution was to revert to the old package version, but that stops us from progressing forward. It's nice to remove dependencies and use built-in options.

Related issues

Fixes:

Manual testing steps

  1. Run yarn storybook.
  2. Visit the component docs page running locally.
  3. Verify that no errors related to theme switching appear in the console.
  4. Ensure the docs page renders properly in both light and dark themes.
  5. Test theme switching using the button added in the component preview.

Screenshots/Recordings

Before

Error Screenshot

Screen.Recording.2024-09-11.at.6.37.07.PM.mov

After

This now allows us to have more than one theme for our components and we can also view both modes at the same time!

Screen.Recording.2024-09-11.at.6.39.07.PM.mov

Here is where I know some people might be upset, but currently the entire application theme is set to users system and no longer tied to the theme switch

Screen.Recording.2024-09-11.at.6.39.07.PM.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pulled and built the branch, run the app, tested code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and/or screenshots.

@garrettbear garrettbear requested a review from a team as a code owner September 12, 2024 00:48
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions github-actions bot added the team-design-system All issues relating to design system in Extension label Sep 12, 2024
Copy link

socket-security bot commented Sep 12, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@storybook/[email protected], npm/[email protected]

View full report↗︎

@garrettbear garrettbear added the area-documentation Issues relating to documentation, in the codebase and off. label Sep 12, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [6824447]
Page Load Metrics (1883 ± 109 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22923871632619297
domContentLoaded152923691864225108
load153723781883227109
domInteractive167538167
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [c2f54b5]
Page Load Metrics (1594 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint21220201469429206
domContentLoaded1498184715817235
load1506188215947837
domInteractive18422873
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.04%. Comparing base (b01f0c9) to head (c2f54b5).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #27086   +/-   ##
========================================
  Coverage    70.04%   70.04%           
========================================
  Files         1435     1435           
  Lines        49920    49920           
  Branches     13980    13980           
========================================
  Hits         34963    34963           
  Misses       14957    14957           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Really nice work 💯 LGTM!

  • Checked storybook docs MDX only pages in light dark theme ✅
  • Checked storybook stories in light,dark,light/dark ✅
  • Checked console for no errors ✅

Copy link

@HowardBraham HowardBraham merged commit 9668df7 into develop Sep 13, 2024
77 of 78 checks passed
@HowardBraham HowardBraham deleted the fix/27001/storybook-docs-not-working branch September 13, 2024 15:26
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 13, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [28dbecd]
Page Load Metrics (1637 ± 82 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22122751446528253
domContentLoaded14332103161915775
load14422207163717282
domInteractive146535178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot metamaskbot added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-documentation Issues relating to documentation, in the codebase and off. release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-design-system All issues relating to design system in Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants