Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

web: upgrade Storybook #36437

Merged
merged 12 commits into from
Jun 7, 2022
Merged

web: upgrade Storybook #36437

merged 12 commits into from
Jun 7, 2022

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented Jun 2, 2022

Context

Upgrading Storybook to the latest version to benefit from goodies released since Oct 14 and eventually enable lazy compilation in development mode. Read more about Storybook Lazy Compilation for Webpack here.

Slack thread for context.

Changes

  1. Replaced deprecated static directories CLI flag with the configuration field.
  2. Replaced ChromaticStory with withChromaticThemes global decorator to snapshot stories in the dark mode in Chromatic automatically. This is necessary because of the story-store API change that we relied on.
  3. Introduced our wrapper around the isChromatic helper to locally test the change described above. Run CHROMATIC=true yarn storybook to test it.
  4. Pinned prettier version to avoid huge diff in this PR. Let's unpin it in a follow-up PR.
  5. Added React context to the Popover component to be able to change the HTML element where it's rendered. Thanks, @vovakulikov, for implementing that! 🙌

New global decorator

The withChromaticThemes decorator renders stories with parameters.chromatic.enableDarkMode === true twice — in light and dark mode. The decorator is enabled only if isChromatic() === true. It's a suggested way of snapshotting the same story with different themes. Trade-offs:

Pros

  1. Simpler code — decorator React component. No need to touch the story store API used previously.
  2. Compatible with the new story store API. We won't need to migrate anything when the Storybook v7 is released.
  3. Two themes on one snapshot —> less number of snapshots -> less spending.

Cons

  1. Dark mode and light mode stories are not isolated — they are rendered in the same React tree. (e.g., that's why we need a way to control the Popover root element).

Test plan

  1. Ensure that Storybook works locally without changes.
  2. Ensure that Chromatic snapshots are as expected.

App preview:

To learn more, check out the client app preview documentation.

@valerybugakov valerybugakov added the frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. label Jun 2, 2022
@valerybugakov valerybugakov self-assigned this Jun 2, 2022
@cla-bot cla-bot bot added the cla-signed label Jun 2, 2022
@vovakulikov vovakulikov force-pushed the vb/storybook-upgrade branch from ff3ca11 to e6fc825 Compare June 2, 2022 13:18
@valerybugakov valerybugakov marked this pull request as ready for review June 3, 2022 06:40
@valerybugakov valerybugakov marked this pull request as draft June 3, 2022 06:40
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jun 3, 2022

Codenotify: Notifying subscribers in CODENOTIFY files for diff cf59b33...f8a41e3.

Notify File(s)
@courier-new client/web/src/enterprise/batches/list/BatchChangeNode.story.tsx
@eseliger client/web/src/enterprise/batches/list/BatchChangeNode.story.tsx

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jun 3, 2022

Codenotify: Notifying subscribers in OWNERS files for diff cf59b33...f8a41e3.

Notify File(s)
@sourcegraph/frontend-platform-devs client/build-config/src/paths.ts
client/wildcard/src/components/Modal/Modal.story.tsx
client/wildcard/src/components/Popover/Popover.tsx
client/wildcard/src/components/Popover/components/PopoverTrigger.tsx
client/wildcard/src/components/Popover/components/floating-panel/FloatingPanel.tsx
client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx
client/wildcard/src/components/Popover/components/popover-tail/PopoverTail.tsx
client/wildcard/src/components/Popover/contexts/internal-context.ts
client/wildcard/src/components/Popover/contexts/public-context.ts
client/wildcard/src/components/Popover/index.ts
client/wildcard/src/components/Popover/story/Popover.story.tsx
client/wildcard/src/components/index.ts

@valerybugakov valerybugakov marked this pull request as ready for review June 3, 2022 10:08
@valerybugakov valerybugakov marked this pull request as draft June 3, 2022 10:08
@valerybugakov valerybugakov marked this pull request as ready for review June 6, 2022 05:47
@valerybugakov valerybugakov requested review from umpox, oleggromov, limitedmage and a team June 6, 2022 05:47
@limitedmage
Copy link
Contributor

Should every component be rendered in both light and dark mode? The UI review only shows some components being rendered in dark mode.

@jasongornall
Copy link
Contributor

jasongornall commented Jun 6, 2022

food for thought.. storybookjs/storybook#16837.. looks like this isn't an issue in 6.5.
this also fixes this ticket! https://github.com/sourcegraph/sourcegraph/issues/34231

from testing it myself looks great!
a nice 2 for 1

@jasongornall
Copy link
Contributor

jasongornall commented Jun 6, 2022

Should every component be rendered in both light and dark mode? The UI review only shows some components being rendered in dark mode.

I think this is intentional. It's basically checking against a chromatic dark variable if it should render the dark mode. We only render darkmode if the story tells us to (like here)

Copy link
Contributor

@jasongornall jasongornall left a comment

Choose a reason for hiding this comment

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

Looks solid from my testing

@limitedmage
Copy link
Contributor

I think this is intentional. It's basically checking against a chromatic dark variable if it should render the dark mode. We only render darkmode if the story tells us to (like here)

Ahh thank you, that's what I was missing here. I didn't understand how some were being rendered in dark mode and some were not based on this changeset alone; turns out it's preexisting code! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants