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

refactor: upgrade storybook #93

Merged
merged 11 commits into from
Nov 4, 2024
Merged

refactor: upgrade storybook #93

merged 11 commits into from
Nov 4, 2024

Conversation

emilyjablonski
Copy link
Contributor

@emilyjablonski emilyjablonski commented Oct 29, 2024

Description

Upgrades Storybook to be able to support some of the documentation requests from SF.

This PR 1) Upgrades Storybook + documentation, and (2) introduces new shared types that are used across components instead of string unions (none of these are breaking changes, but having importable types has come up a few times as well).

Each code change unrelated to this has a comment on it. This includes two separate changes that are breaking, which is to rename the highlight variant of Button to warn, and the in-process variant of Tag to warn, to match the tokens and designs. (UPDATE: Now also includes renaming instances of accent to highlight!)

How Can This Be Tested/Reviewed?

The huge diff is mostly in the lock file.

  • Changes in mdx and stories files are for managing the upgrade, improving organization, and enabling code samples
  • Changes in Storybook configuration files are for managing the upgrade
  • Changes in .tsx files are for improving typing

Compare the old Storybook to the deploy preview here to see changes and ensure things are looking good! The Button and Tag stories will be the only ones w the naming adjustment to warn.

There are now

  • Code samples for every story
  • Links to Zeroheight

Checklist:

  • I have added QA notes to the issue with applicable URLs
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have exported any new components
  • My commit message(s) and PR title are polished in the conventional commit format, and any breaking changes are indicated in the message and are well-described

Copy link

netlify bot commented Oct 29, 2024

Deploy Preview for storybook-ui-seeds ready!

Name Link
🔨 Latest commit 7c7add4
🔍 Latest deploy log https://app.netlify.com/sites/storybook-ui-seeds/deploys/67229a3601b7680008b3ca93
😎 Deploy Preview https://deploy-preview-93--storybook-ui-seeds.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

&[data-variant="highlight"] {
background-color: var(--seeds-color-highlight);
border-color: var(--seeds-color-highlight);
&[data-variant="warn"] {
Copy link
Contributor Author

@emilyjablonski emilyjablonski Oct 29, 2024

Choose a reason for hiding this comment

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

Updated the naming here to warn - we don't have a highlight in the Figma tokens, and the colors are the same (confirmed with design)

@@ -153,7 +154,8 @@
--seeds-bg-color-surface: var(--seeds-color-white);
--seeds-bg-color-muted: var(--seeds-color-gray-100);
--seeds-bg-color-disabled: var(--seeds-color-gray-450);
--seeds-bg-color-inverse: var(--seeds-colors-gray-800);
--seeds-bg-color-inverse: var(--seeds-color-gray-800);
Copy link
Contributor Author

@emilyjablonski emilyjablonski Oct 29, 2024

Choose a reason for hiding this comment

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

Fixes a typo

@@ -1,39 +1,36 @@
:root {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just organized this file and moved strings around

&[data-variant="in-process"] {
background-color: var(--seeds-color-highlight-light);
color: var(--seeds-color-highlight-darker);
&[data-variant="warn"] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the naming here to warn - we don't have an in-process in the Figma tokens, and the colors are the same (confirmed with design)

@emilyjablonski emilyjablonski marked this pull request as ready for review October 29, 2024 03:54
@jaredcwhite
Copy link
Collaborator

@emilyjablonski I've found a difference in the URLs between the old system and Storybook 8 for the documentation pages.

OLD URL: /iframe.html?viewMode=docs&id=layout-grid--grid-example&args=

NEW URL: /iframe.html?viewMode=docs&id=layout-grid--docs&globals=

This is only an issue with regards to the zeroheight docs embeds. We'd have to update all of them manually once this goes to prod. Or I wonder if there's a way to set up a redirect?

@emilyjablonski
Copy link
Contributor Author

@jaredcwhite Good call out! I was anticipating a followup to manually update the embeds 🫡

Copy link
Collaborator

@jaredcwhite jaredcwhite left a comment

Choose a reason for hiding this comment

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

Left some feedback for you. I'm glad we'll be on Storybook 8 now, looks really good!

index.ts Show resolved Hide resolved
src/actions/Button.tsx Show resolved Hide resolved
<Meta of={stories} />

<Title />
<Subtitle>Button components are interactive elements that trigger an action when clicked.</Subtitle>
Copy link
Collaborator

Choose a reason for hiding this comment

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

So…this looks really nice in the new Storybook theme, but it'll also show up in the Props embeds in zeroheight. I guess that's fine, but something to keep in mind so it's not too repetitive when viewing in zeroheight.

src/blocks/__stories__/Alert.stories.tsx Show resolved Hide resolved
src/blocks/__stories__/Message.stories.tsx Show resolved Hide resolved
src/forms/__stories_/CheckboxGroup.docs.mdx Show resolved Hide resolved
src/global/tokens/colors.scss Outdated Show resolved Hide resolved
@emilyjablonski
Copy link
Contributor Author

Post-merge, I will 1) Post the breaking changes in our consumers channel and 2) Update the Zeroheight Storybook embeds!

Copy link
Collaborator

@jaredcwhite jaredcwhite left a comment

Choose a reason for hiding this comment

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

📚

@emilyjablonski emilyjablonski merged commit 65c743e into main Nov 4, 2024
5 checks passed
Copy link

github-actions bot commented Nov 4, 2024

🎉 This PR is included in version 1.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants