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(storybook): update v1 syntax to use Storybook v6 #568

Merged

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Jul 18, 2024

Follow-up to #432 (comment) and make the v1 code Storybook v7 compatible. This is just one step toward the upgrade, but perhaps the most breaking one.
Follow-up to #567 "Future Work" part 1.

Motivation

  • while the deps used Storybook v6, the configuration and stories themselves were still on a legacy v5 format
    • v6 had backward compat for v5, but v7 does not, so this needs upgrading/migrating
      • or well, v7 has some legacy mode for it that can be enabled with some config, but it is entirely gone in v8

Modifications

Verification

yarn start works the same as it did in #567

Notes to Reviewers

This requires #567 to work and so is built on top of it. Please do not merge until that is merged first!
Rebased on top

- while the deps used Storybook v6, the configuration and stories themselves were still on a legacy v5 format
  - v6 had backward compat for v5, but v7 does not, so this needs upgrading/migrating
    - or well, v7 has some legacy mode for it that can be enabled with some config, but it is entirely gone in v8
  - migrate `storiesOf` to CSF per https://storybook.js.org/docs/7/migration-guide#storiesof-to-csf
    - then had to do a bunch of manual changes to get back mostly the same previous indentation (with some minor differences where there were mistakes or inconsistencies)
      - tried to keep the diff as small as possible
    - CSF is still valid through to latest v8

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- modified version of v2's config

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- `ts-loader` is necessary to handle the `import type` syntax
  - custom `tsconfig.json` doesn't seem necessary though
- use same SASS config
- plain CSS config not needed though bc PostCSS already runs by default

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- the conventional format
  - also had some problems finding stories without this change

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- per https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#hoisted-csf-annotations
  - migrate `.story.name` -> `.storyName`
    - not sure why the automigration didn't do this and instead used the deprecated name 😕

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- remove `ts-loader` as not necessary
- plus some settings for Storybook's TS loader preset

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the chore-update-v1-storybook-v6-syntax branch from ea006e0 to 8db46bd Compare August 22, 2024 21:18
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 marked this pull request as ready for review August 22, 2024 21:54
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Local test lgtm

@crenshaw-dev crenshaw-dev merged commit d1f1d44 into argoproj:master Aug 23, 2024
5 checks passed
@agilgur5 agilgur5 deleted the chore-update-v1-storybook-v6-syntax branch August 23, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants