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

build: Upgrade Storybook to 5.2 #267

Merged
merged 26 commits into from
Nov 4, 2019

Conversation

sahlhoff
Copy link
Contributor

@sahlhoff sahlhoff commented Oct 15, 2019

Hi all, you'll notice two storybook changes in this PR: Avatar and Card. Avatar is using the legacy storiesOf api but with addParameters({component: <component>}). Card is using the new CSF format.

Some notes:

I'm unable to get CSF to work with the source-loader. The source-loader is able to parse the legacy stories (and show the code) but is having issues showing the CSF code. There's similar issues on github.

You'll notice for both the Card and Avatar story we are able to parse the props in the props section. However this appears to be just the default props from the component. I did have some success using the docgenLoader – you'll see commented out in .storybook/webpack.config.js. This however had a severe impact on build performance and also LOTS of props got passed because in most cases we are extending HTMLAttributes.

The refactor to CSF is going to be a pretty large change to the codebase. CSF requires that there are only one component per file– this means potentially we could at least see double of the storybook files in components like button and form-field. I've got a lot of these changes stashed locally and I can follow up with them if we can figure out the webpack issues / source-loader.

Any thoughts or help, greatly appreciated! 🙏

Checklist

  • branch has been rebased on the latest master commit
  • tests are changed or added
  • yarn test passes
  • all (dev)dependencies that the module needs is added to its package.json
  • code has been documented and, if applicable, usage described in README.md
  • module has been added to canvas-kit-react and/or canvas-kit-css universal modules, if
    applicable
  • design approved final implementation
  • a11y approved final implementation
  • code adheres to the API & Pattern guidelines

Additional References

Storybook's recommended docs configuration: https://github.com/storybookjs/storybook/tree/next/addons/docs#manual-configuration

Performance

image

image

@NicholasBoll
Copy link
Member

It looks like the Chromatic CI check is failing on yarn build-storybook

@lychyi lychyi moved this from To do to In progress in Current Sprint (7/20 - 8/9) Oct 15, 2019
@NicholasBoll
Copy link
Member

If we don't want to list out all the properties on the host Element, it looks like props can be filtered out: storybookjs/storybook#8104 (comment)

@sahlhoff
Copy link
Contributor Author

@NicholasBoll this should be able yarn build-storybook after we upgrade to emotion 10

@@ -0,0 +1,65 @@
const docGen = require('react-docgen-typescript');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NicholasBoll NicholasBoll added the dependencies Pull requests that update a dependency file label Oct 23, 2019
@NicholasBoll
Copy link
Member

@anicholls There was some dependency mismatches, so we simply updated the yarn.lock file will all the latest changes and that fixed the issue, but added a lot of diffs to the yarn.lock file. Is this okay or should we narrow it down by updating the package.json file as well?

@anicholls anicholls changed the title WIP: Storybook upgrade 5.2 build: Upgrade Storybook to 5.2 Oct 25, 2019
@lychyi
Copy link
Contributor

lychyi commented Oct 25, 2019

@NicholasBoll Changes were manually introduced to yarn.lock?

@anicholls
Copy link
Contributor

@NicholasBoll I told you 🤣

@sahlhoff sahlhoff marked this pull request as ready for review October 28, 2019 17:36
@NicholasBoll
Copy link
Member

@sahlhoff @anicholls What happened here? https://www.chromaticqa.com/snapshot?appId=5d854c26ba934e0020f5e98a&id=5db78ba417a1ee0020846163

It looks like now there is a background image? It wasn't showing before?

@sahlhoff
Copy link
Contributor Author

@NicholasBoll just looking at the resources panel in chrome– looks like previously it was incorrectly getting base64ed compared to current build. The story css has it explicitly set so going to assume we want it here.

image

@lychyi lychyi mentioned this pull request Oct 29, 2019
31 tasks
@NicholasBoll NicholasBoll merged commit ecc5a1b into Workday:master Nov 4, 2019
Current Sprint (7/20 - 8/9) automation moved this from In progress to Done Nov 4, 2019
@anicholls anicholls mentioned this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants