chore(deps): update storybook to v6 #1638

teimurjan · 2020-10-22T08:43:49Z

Description

Updating Storybook from v5 to v6 is a pretty big thing. It includes some breaking changes. In terms of this PR, all the breaking changes are taken into account:

Typescript is working with the storybook out of the box.
Changed the stories' hierarchy.
Added core-js library.
Added main.js file.

Problems

⚠️ 1 visual test is broken for an unknown reason. Would like to have a hand for it, cause I tried everything I knew.

Investigation

Reason: The problem was caused by the conflict of the new story layout and our own body styles. In v5 there was no padding for layout at all, so there was no conflict. In v6 there is a default padding equals to 1rem. It can be avoided by using export const parameters = { layout: 'padded' } in preview.js file. However, when we run visual testing we don't use Storybook's index.html with all the stories. We use the iframe.html with the only story in it. The body inside this iframe has the padding I've mentioned above. So the trouble is layout styles are controlled when iframe.html is used inside the index.html. When it's rendered separately the default style is used.

Solution: We apply our own styles because of the better look of the image snapshots. However, now these styles are applied by default in Storybook (the only difference is: we use margin: 10px and Storybook uses padding: 1rem). So I got rid of our own styles and made the layout property to be padded in the parameters. It affected all the images snapshots but I manually compared every diff and can claim that nothing is broken.

How to test

Check out the new storybook on this PR's temploy.

Screenshots

Before.	After.

Review

Read CONTRIBUTING.md and Component API principles
Make sure you've converted all js/jsx file into ts/tsx in your PR
Annotate all props in component with documentation
Create examples for component
Ensure that deployed demo has expected results and good examples
Ensure that unit tests pass by running yarn test
Ensure that visuals tests pass by running yarn test:visual. If not - check the documentation how to fix visual tests

PR commands

List of available commands:

@toptal-bot run all - Run whole pipeline
@toptal-bot run danger - Danger checks
@toptal-bot run lint - Run linter
@toptal-bot run test - Run jest
@toptal-bot run build - Check build
@toptal-bot run test:visual or @toptal-bot run visual - Run visual tests
@toptal-bot run deploy:documentation - Deploy documentation
@toptal-bot run package:alpha-release - Release alpha version

Bumps [@storybook/react](https://github.com/storybookjs/storybook/tree/HEAD/app/react) from 5.3.18 to 6.0.26. - [Release notes](https://github.com/storybookjs/storybook/releases) - [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md) - [Commits](https://github.com/storybookjs/storybook/commits/v6.0.26/app/react) Signed-off-by: dependabot-preview[bot] <[email protected]>

havenchyk · 2020-10-22T09:33:56Z

🥇

.storybook/stories/Contributing.tsx

denieler · 2020-10-28T08:44:29Z

Still not that cool as it was before 😁 Maybe we can write an addon to do that job?

teimurjan · 2020-10-28T10:49:48Z

@denieler not sure about an addon, but I think it's possible. However, I think that the current version is OK before we find a stable solution for customizing the sidebar. I'd do it within another story.

denieler · 2020-10-28T10:52:09Z

@denieler not sure about an addon, but I think it's possible. However, I think that the current version is OK before we find a stable solution for customizing the sidebar. I'd do it within another story.

I think we can't deploy it like this to picasso.toptal.net. It can confuse other devs, who are using Picasso site every day

denieler · 2020-10-29T08:22:30Z

This sounds a bit scary to merge all screenshot images updated 😨

teimurjan · 2020-10-29T09:04:04Z

@denieler Getting rid of custom body styles is an advantage that worth it. We should assume that no component code has been touched (except types) meaning the components are the same. It took me 10 minutes to check all the screenshots.

.storybook/components/PicassoBook/Book.js

.storybook/components/PicassoBook/Page.js

.storybook/components/PicassoBook/components/PropsTable/PropTypeTableCell.tsx

denieler · 2020-10-29T09:19:19Z

.storybook/utils/documentation-generator.ts

@@ -23,8 +23,8 @@ export interface PropDocumentationMap {
 }

 export interface Documentable {
-  displayName: string
-  __docgenInfo: any
+  displayName?: string


why do we need this change?

When using forwardRef it returns ForwardRefExoticComponent. In ForwardRefExoticComponent displayName is an optional string. Indicator is a good example of such a component. So. when passing Indicator to addComponentDocs there is a TS error saying that displayName should be string

why this issue came up only now?

It happened after TS upgrade

probably not TS upgrade but rather react types?

Not sure. I think the react types are the same. A lot of TS issues were ignored before the update.

packages/picasso-charts/tsconfig.build.json

packages/picasso-lab/src/MediaSkeletonLoader/MediaSkeletonLoader.tsx

packages/picasso/src/Indicator/Indicator.tsx

tsconfig.json

.storybook/tsconfig.json

denieler · 2020-10-30T08:39:28Z

Looks like direct links stopped working

teimurjan · 2020-10-30T09:53:12Z

@denieler committed the fix

borisyordanov · 2020-10-30T10:30:48Z

but I manually compared every diff and can claim that nothing is broken.

🥇

teimurjan · 2020-10-30T12:09:51Z

@toptal-bot run package:alpha-release

teimurjan · 2020-11-02T06:03:23Z

@toptal-bot run package:alpha-release

teimurjan · 2020-11-03T11:15:15Z

@toptal-bot run package:alpha-release

havenchyk · 2020-11-03T13:51:27Z

@toptal-bot run package:alpha-release

vedrani

I like it, much faster than SP5, do we have some problems to discuss before merge on which I could concentrate?

vedrani · 2020-11-03T14:21:41Z

.storybook/preview-head.html

-    // we have left here 10px margin for visual tests
-    // to make prettier screenshots and to avoid hiding
-    // box shadows from the captured screenshots for some components
-    document.body.style = 'padding: 0; margin: 10px !important;'


Do we need this padding? Or something changed in SB6?

SB6 adds padding by default, so I've removed it

vedrani · 2020-11-03T14:23:41Z

package.json

@@ -133,5 +130,8 @@
    "lodash": "^4.17.12",
    "**/marksy/marked": "^0.6.0",
    "**/npx/npm": "^5.7.1"
+  },
+  "dependencies": {
+    "@storybook/react": "^6.0.27"


Should it also be in dev dependencies?

but actually for root package json it doesn't really matter

Interesting, how react is used during dev?

@OleksandrNechai Storybook needs react to render the JSX

havenchyk · 2020-11-03T14:48:08Z

@toptal-bot run package:alpha-release

havenchyk · 2020-11-03T15:03:00Z

@toptal-bot run package:alpha-release

toptal-devbot · 2020-11-03T15:26:58Z

Your alpha package is ready 🎉
yarn add @toptal/[email protected]
yarn add @toptal/[email protected]
yarn add @toptal/[email protected]
yarn add @toptal/[email protected]
yarn add @toptal/[email protected]
yarn add @topkit/[email protected]

teimurjan · 2020-11-04T06:15:07Z

@toptal-bot run package:alpha-release

packages/picasso-lab/tsconfig.build.json

havenchyk

I'm still afraid of typescript related changes, let me try it out locally carefully to make sure nothing is broken

havenchyk · 2020-11-04T11:48:19Z

@teimurjan consider going one step forward

diff --git a/tsconfig.build.json b/tsconfig.build.json
index 62335ffc..b7a57514 100644
--- a/tsconfig.build.json
+++ b/tsconfig.build.json
@@ -1,13 +1,7 @@
 {
   "extends": "./tsconfig.json",
   "compilerOptions": {
-    "outDir": "dist-package",
-    "paths": {
-      "*": ["@types/*"],
-      "~/*": ["./*"],
-      "@toptal/picasso/*": ["node_modules/@toptal/picasso/src/*"],
-      "@toptal/*": ["node_modules/@toptal/*/src"]
-    }
+    "outDir": "dist-package"
   },
   "exclude": [
     "**/*.example.jsx",
diff --git a/tsconfig.json b/tsconfig.json
index 8d3df2e4..688ffca6 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -16,20 +16,13 @@
     "baseUrl": ".",
     "typeRoots": ["node_modules/@types", "@types"],
     "paths": {
-      "@toptal/*": ["packages/*/src"],
-      "@topkit/analytics-charts": ["packages/topkit-analytics-charts/src"],
-      "@topkit/analytics-charts/*": ["packages/topkit-analytics-charts/src/*"],
-      "@toptal/picasso/*": ["packages/picasso/src/*"],
-      "@toptal/picasso-lab/*": ["packages/picasso-lab/src/*"],
-      "@toptal/picasso-shared/*": ["packages/shared/src/*"],
-      "@toptal/picasso-shared": ["packages/shared/src"],
-      "@toptal/picasso-forms/*": ["packages/picasso-forms/src/*"],
-      "@toptal/picasso-charts/*": ["packages/picasso-charts/src/*"],
       "*": ["@types/*"],
-      "~/*": ["./*"]
+      "~/*": ["./*"],
+      "@toptal/picasso/*": ["node_modules/@toptal/picasso/src/*"],
+      "@toptal/*": ["node_modules/@toptal/*/src"]
     },
     "types": ["node", "jest", "@testing-library/jest-dom"]
   },
-  "include": ["packages/*/src"],
+  "include": ["packages/*/src", "./cypress/integration", "./node_modules/cypress"],
   "exclude": ["packages/*/dist-package"]
 }

OleksandrNechai · 2020-11-04T12:17:04Z

.storybook/addons/menu-expander/index.js

+  `
+}
+
+const withSavingInitialPath = (cb, api) => {


Usually, we don't use abbreviations

Instead of the callback, maybe it makes sense to return Promise?

Not possible. The callback is used not as an end function, but it's called in between the steps. It'll be removed once storybookjs/storybook#12980 is merged.

OleksandrNechai

Well, I myself also have positive impressions from what I have seen so far. Works fast, looks beautiful 👍
Code is clean 👍 although I am not sure about exactly what each line does.
Tested

locally
deployment
a few packages randomly

I could not see the content of the package (Explore tab) https://www.npmjs.com/package/@toptal/picasso/v/4.109.8-alpha-update-storybook-to-v6.1952
I could not make these packages work in code sandbox https://codesandbox.io/s/jovial-meninsky-n7c52?file=/package.json

I am now not sure if we have not broken package deployment here.

teimurjan · 2020-11-04T15:04:46Z

@toptal-bot run all

toptal-devbot · 2020-11-04T15:45:30Z

🎉 Last commit is successfully deployed 🎉

Demo is available on:

https://picasso.toptal.net/update-storybook-to-v6/

Your davinci-bot 🚀

havenchyk · 2020-11-06T11:14:47Z

tsconfig.json

@@ -27,8 +27,9 @@
      "@toptal/picasso-charts/*": ["packages/picasso-charts/src/*"],
      "*": ["@types/*"],
      "~/*": ["./*"]
-    }
+    },
+    "types": ["node", "jest", "@testing-library/jest-dom", "cypress"]


I doubt it 😄 https://www.staging-typescript.org/tsconfig#types

By default all visible ”@types” packages are included in your compilation. Packages in node_modules/@types of any enclosing folder are considered visible. For example, that means packages within ./node_modules/@types/, ../node_modules/@types/, ../../node_modules/@types/, and so on.

If types is specified, only packages listed will be included in the global scope

There is an error when not using types:

cypress/integration/ModalAndTooltip.spec.ts:17:5 - error TS2304: Cannot find name 'cy'. 17 cy.get('[data-testid="datepicker"]').should('have.value', '11-03-2020')

Previously the problem was solved by adding "./node_modules/cypress" to include. In this case expect function in Jest tests is considered like expect from cypress.

Property 'toMatchSnapshot' does not exist on type 'Assertion'.ts(2339)

That's why the types are required.

havenchyk

Looks great, except types fields which I doubt about

* chore(deps-dev): bump @storybook/react from 5.3.18 to 6.0.26 Bumps [@storybook/react](https://github.com/storybookjs/storybook/tree/HEAD/app/react) from 5.3.18 to 6.0.26. - [Release notes](https://github.com/storybookjs/storybook/releases) - [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md) - [Commits](https://github.com/storybookjs/storybook/commits/v6.0.26/app/react) Signed-off-by: dependabot-preview[bot] <[email protected]> * chore: migrate storybook to v6 * chore: add node types to tsconfig * chore: get rid of tsconfig.build paths * chore: fix menu expander * chore: fix storybook links * chore: update snapshots Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

teimurjan added the no-jira label Oct 22, 2020

teimurjan changed the title ~~Update storybook to v6~~ chore(deps): update storybook to v6 Oct 23, 2020

teimurjan marked this pull request as ready for review October 23, 2020 05:39

teimurjan requested a review from a team as a code owner October 23, 2020 05:39

teimurjan removed the no-jira label Oct 23, 2020

teimurjan force-pushed the update-storybook-to-v6 branch from f9a4136 to b5d4a90 Compare October 26, 2020 08:39

denieler reviewed Oct 26, 2020

View reviewed changes

.storybook/stories/Contributing.tsx Show resolved Hide resolved

chore: migrate storybook to v6

c88fbd5

teimurjan force-pushed the update-storybook-to-v6 branch from baef09f to c88fbd5 Compare October 28, 2020 07:03

Teimur Gasanov added 2 commits October 28, 2020 16:32

chore: fix type errors

0c378d1

chore: add node types to tsconfig

76a0cb5

chore: get rid of tsconfig.build paths

bc721b2

denieler reviewed Oct 29, 2020

View reviewed changes

chore: fix menu expander

e935208

teimurjan force-pushed the update-storybook-to-v6 branch from 20673fe to e935208 Compare October 30, 2020 06:11

Merge branch 'master' into update-storybook-to-v6

01211ce

denieler reviewed Oct 30, 2020

View reviewed changes

.storybook/tsconfig.json Show resolved Hide resolved

chore: fix storybook links

93a9c2b

chore: update davinci

b2bd1a4

vedrani approved these changes Nov 3, 2020

View reviewed changes

Merge branch 'master' into update-storybook-to-v6

467cf63

chore: update tsconfig build

d855ada

Teimur Gasanov added 2 commits November 4, 2020 14:27

Merge branch 'master' into update-storybook-to-v6

0d022a6

Merge branch 'master' into update-storybook-to-v6

9e7f5c1

OleksandrNechai reviewed Nov 4, 2020

View reviewed changes

packages/picasso-lab/tsconfig.build.json Show resolved Hide resolved

havenchyk suggested changes Nov 4, 2020

View reviewed changes

OleksandrNechai reviewed Nov 4, 2020

View reviewed changes

OleksandrNechai approved these changes Nov 4, 2020

View reviewed changes

chore: update tsconfig to include cypress types

675e280

havenchyk reviewed Nov 6, 2020

View reviewed changes

havenchyk approved these changes Nov 6, 2020

View reviewed changes

teimurjan merged commit c800c1b into master Nov 6, 2020

teimurjan deleted the update-storybook-to-v6 branch November 6, 2020 11:56

teimurjan restored the update-storybook-to-v6 branch November 16, 2020 06:23

teimurjan deleted the update-storybook-to-v6 branch November 16, 2020 06:24

chore(deps): update storybook to v6 #1638

chore(deps): update storybook to v6 #1638

Conversation

teimurjan commented Oct 22, 2020 • edited Loading

Description

Problems

Investigation

How to test

Screenshots

Review

havenchyk commented Oct 22, 2020

denieler commented Oct 28, 2020

teimurjan commented Oct 28, 2020

denieler commented Oct 28, 2020

denieler commented Oct 29, 2020

teimurjan commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denieler commented Oct 30, 2020 • edited Loading

teimurjan commented Oct 30, 2020

borisyordanov commented Oct 30, 2020

teimurjan commented Oct 30, 2020

teimurjan commented Nov 2, 2020

teimurjan commented Nov 3, 2020

havenchyk commented Nov 3, 2020

vedrani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havenchyk commented Nov 3, 2020

havenchyk commented Nov 3, 2020

toptal-devbot commented Nov 3, 2020

teimurjan commented Nov 4, 2020

havenchyk left a comment

Choose a reason for hiding this comment

havenchyk commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OleksandrNechai left a comment • edited Loading

Choose a reason for hiding this comment

teimurjan commented Nov 4, 2020

toptal-devbot commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havenchyk left a comment

Choose a reason for hiding this comment

teimurjan commented Oct 22, 2020 •

edited

Loading

denieler commented Oct 30, 2020 •

edited

Loading

OleksandrNechai left a comment •

edited

Loading