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

Upgrade to storybook 6 #35

Closed
aaronBery opened this issue Oct 14, 2020 · 6 comments · Fixed by #791
Closed

Upgrade to storybook 6 #35

aaronBery opened this issue Oct 14, 2020 · 6 comments · Fixed by #791
Assignees

Comments

@aaronBery
Copy link
Contributor

This was initially flagged in #28 which references a handy migration guide on medium.

There are breaking changes associated with the upgrade and also decisions on best practice to be made e.g. which story format to adopt

@elenagarrone
Copy link
Contributor

Also, you will have to consider the dsm storybook npm package. I quickly had a look at this when doing the dependencies upgrades a while ago and it didn't seem that straight forward. It might have changed in the past month 🤞

@andy-polhill
Copy link
Contributor

I'm not happy with that DSM package and the amount of dependencies it has, which potentially hold us back on updates. A new version did get published two weeks ago, but from what I can see the release notes aren't published.
https://www.npmjs.com/package/@invisionapp/dsm-storybook

I wouldn't be opposed to looking at alternatives, to me it would seem simpler if we just iframed the storybook page into the DSM.

@andy-polhill
Copy link
Contributor

I had a few goes at playing around with this recently, there are some pretty big changes.

I made better progress by removing the existing .storybook' directory and then just running a single story. The Alert` component was a good candidate for simplicity. From what I could see though, content projection prevents us from using the simplest approach.

I think Storybook 6 might reduce lots of the documentation we need to do. We could consider removing the html only support from the docs.

@andy-polhill
Copy link
Contributor

@paul-request what were your findings in the end? did you have any further ideas on how to tackle it?

@paul-request
Copy link
Contributor

paul-request commented Jan 8, 2021

So I didn't get as far as I would have liked, but I have managed to upgrade and get it running with an Alert component.

These are the main things to take away I think:

  • Automatic documentation is achieved by using Compodoc internally, which has turned out to be massively disappointing. It doesn't do much more than add descriptions. So actually we would still be required to add documentation for things manually like we do now.
  • Compodoc also adds private variables into the docs and controls, so these would have to be ignored explicitly using a comment (not ideal). Update: it can be ignored by passing --disablePrivate flag to the compodoc nam script.
  • Knobs is replaced by Controls, which has a new API to use. Seems fine, but there is an outstanding bug where changing the controls in the docs tab doesn't update the example. (Addon-docs: DocsPage Controls don't update iframe stories storybookjs/storybook#11908). Update: this has been merged into their next branch so will be released when version of 6.2 is released (currently in alpha).

Still todo:

  • There is a new feature called DocsPage, which I need to look into that further. At first glance, it looks like we can just use it by creating a markdown file, instead of exporting strings of markdown from ts. Update: Have tried a number of approaches, but have not been able to get the mdx version of CSF working :-(
  • Look at a more complex component with variants and see if there are other things that may not work without further configuration/changes.
  • Investigate DSM integration.

Once I have had a look at the documentation/notes side of things, I will raise a draft PR to demo the changes needed for the Alert component

@andy-polhill andy-polhill pinned this issue Jan 18, 2021
andy-polhill added a commit to andy-polhill/canopy that referenced this issue Jan 18, 2021
Temporarily removing storybook from the dependabot updates to reduce the amount of noise. Once we
have migrated to storybook 6 we should revert this change.

re Legal-and-General#35
andy-polhill added a commit to andy-polhill/canopy that referenced this issue Jan 18, 2021
Temporarily removing storybook from the dependabot updates to reduce the amount of noise. Once we
have migrated to storybook 6 we should revert this change.

re Legal-and-General#35
owensgit pushed a commit that referenced this issue Jan 18, 2021
Temporarily removing storybook from the dependabot updates to reduce the amount of noise. Once we
have migrated to storybook 6 we should revert this change.

re #35
paul-request referenced this issue in paul-request/canopy Feb 10, 2021
BREAKING CHANGE: Chanhges to the eway docs are written

#35
@owensgit
Copy link
Contributor

A new branch has been created in the main repo called sb-update-v6.

So far it just contains updates to the package.json to add v6.3.11 of Storybook.

Please open a new PRs into sb-update-v6.

Paul's original PR can be used as a frame of reference to help with changes needed.

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 a pull request may close this issue.

5 participants