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

docs(Storybook): upgrade to Storybook V8 #639

Merged
merged 13 commits into from
May 7, 2024
Merged

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented May 6, 2024

Description

Migrate from Storybook@7.x.x to Storybook@8.0.10

Detail

  • Converts *.stories.mdx to CSF3 + MDX using script
  • Applies changes from v8 migration script
  • Fixes linting and TS issues
  • Cleans-up stories
  • Upgrades to getColorV8 from @zendeskgarden/react-theming to avoid future breaking-changes

Screen Shot 2024-05-05 at 7 16 37 PM
Screen Shot 2024-05-05 at 7 17 02 PM

Checklist

  • 🌐 demo is up-to-date (npm start)
  • 💂‍♂️ includes new unit tests
  • ♿ tested for WCAG 2.1 AA compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@ze-flo ze-flo requested a review from a team as a code owner May 6, 2024 05:18
@coveralls
Copy link

coveralls commented May 6, 2024

Coverage Status

coverage: 95.992%. remained the same
when pulling 2d491dd on ze-flo/storybook-v8-migration
into 2fa495f on main.

Comment on lines +20 to +23
function getAbsolutePath(value) {
return dirname(require.resolve(join(value, 'package.json')));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following recommendations for monorepo environments.

@@ -36,7 +36,8 @@
"files": ["*.stories.tsx", "packages/*/demo/**/*"],
"rules": {
"react/button-has-type": "off",
"react/no-array-index-key": "off"
"react/no-array-index-key": "off",
"func-name-matching": "off"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To support React hooks and useArgs within story's render field, the docs recommend using this syntax to avoid linting errors.

{
  render: function Render (args) {...}
  ...
}

Disabling func-name-matching is preferable to turning off react-hooks/rules-of-hooks.

Copy link
Member

Choose a reason for hiding this comment

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

From that link...

To avoid linting issues, it is recommended to use a function with a capitalized name. If you are not concerned with linting, you may use an arrow function.

Are you sure they aren't referring to their own Storybook linter (which we don't use)? If so, then Garden would definitely prefer arrow functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That applies to our eslint config as well. To be clear, we can still use arrow functions and should do so when not using hooks.

func-name-matching expects { render: function render {...} }, however react-hooks/rules-of-hooks requires that a function be PascalCased when using hooks. Disabling react-hooks/rules-of-hooks can lead to bugs but turning off func-name-matching won't. That's why I think it's a better choice.

The alternative is to move that function elsewhere.

const Controlled: Story = {
  render: function Render (args) {...}
  ...
}

Becomes

export const Controlled: Story = {
  render: Render,
  ...
}

function Render (args) {...}

But I think it's more cumbersome and would reduce readability.

Copy link
Member

Choose a reason for hiding this comment

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

ic – yeah, then what you have is the right trade-off. Thanks for the explanation.

@@ -8,7 +8,7 @@
import React, { StrictMode } from 'react';
import { createGlobalStyle } from 'styled-components';
import { create } from '@storybook/theming/create';
import { DEFAULT_THEME, getColor } from '@zendeskgarden/react-theming';
import { DEFAULT_THEME, getColorV8 } from '@zendeskgarden/react-theming';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future-proofing until v9 upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'd prefer to leave this as getColor so that it is forced to be addressed when this repo renovates up to v9.

};

export const Controlled: Story = {
render: function Render(args) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the docs recommended syntax when using hooks within the render fn.

Copy link
Member

@jzempel jzempel May 7, 2024

Choose a reason for hiding this comment

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

ditto .. can Garden opt for arrow functions? Or are they referring to the hooks linter?

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Hard to tell if there's more magic in v8 or if there was a lot of MDX magic in v7 that is now being passed along to the consumer 😅 . But solid work here @ze-flo

package.json Outdated
Comment on lines 45 to 52
"@storybook/addon-a11y": "^8.0.9",
"@storybook/addon-essentials": "^8.0.9",
"@storybook/addon-mdx-gfm": "^8.0.9",
"@storybook/addon-styling": "1.3.7",
"@storybook/react": "7.6.17",
"@storybook/react-webpack5": "7.6.17",
"@storybook/addon-webpack5-compiler-babel": "^3.0.3",
"@storybook/blocks": "^8.0.9",
"@storybook/react": "^8.0.9",
"@storybook/react-webpack5": "^8.0.9",
Copy link
Member

Choose a reason for hiding this comment

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

All dependencies should be exact versions – no ^ ranges

@@ -0,0 +1,19 @@
import { Meta, Controls, Canvas, Story, Markdown } from '@storybook/blocks';
import README from '../README.md';
import * as FocusvisibleStories from './focusvisible.stories';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import * as FocusvisibleStories from './focusvisible.stories';
import * as FocusVisibleStories from './focusvisible.stories';

[nit]

@@ -0,0 +1,19 @@
import { Meta, Controls, Canvas, Story, Markdown } from '@storybook/blocks';
import README from '../README.md';
import * as ScrollregionStories from './scrollregion.stories';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import * as ScrollregionStories from './scrollregion.stories';
import * as ScrollRegionStories from './scrollregion.stories';

[nit]

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

👌🏻 Appreciate your diligence on the component signatures in the Story objects. Came together really nice!

'@storybook/addon-a11y'
getAbsolutePath('@storybook/addon-a11y'),
getAbsolutePath('@storybook/addon-mdx-gfm'),
'@storybook/addon-webpack5-compiler-babel'
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you end up having any insights into why the SWC compiler was having issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what the root cause is. I'll have to dig deeper on that in a follow-up PR.

@ze-flo
Copy link
Contributor Author

ze-flo commented May 7, 2024

👌🏻 Appreciate your diligence on the component signatures in the Story objects. Came together really nice!

Everything is type-safe now. That uncovered a few errors that are now fixed. 🙌

@ze-flo ze-flo merged commit 778fdef into main May 7, 2024
4 checks passed
@ze-flo ze-flo deleted the ze-flo/storybook-v8-migration branch May 7, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants