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 to storybook 6.3 and convert to workspace #1231

Merged
merged 32 commits into from Jul 29, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Jul 7, 2021

Summary

Upgrade storybook/ to storybook@^6.3.0 and convert to yarn workspace.

Details

List of changes

  • Convert storybook and storybook-docs to yarn workspace.
  • Upgrade to storybook v6.3.0, including various config file and story param changes. See storybook/MIGRATION.md for all migration changes.
  • Move all storybook dependencies from top-level package.json to respective workspaces.
  • Move stories/ to storybook/stories/.
  • Refactor relative links to src/ in stories/ to @elastic/charts.
  • Create replacement decorator for now deprecated @storybook/addon-info to show info inline
  • Removestorybook-docs, docs to be used in new kibana docs in future changes.
  • Update VRT server with new globals API.

List of changes to stories

This has a big diff to the stories where every story is changed. The changes include:

  • Replacing relative links to point at @elastic/charts
  • Adding useBaseTheme hook to all stories to allow for global theme and background color switcher
  • Removing now unneeded story-chart class on Charts
  • Update storybook parameters api from Example.story.parameters to Example.parameters
  • Remove all dark/light theme logic in favor of global theme switcher
  • Disable background addon for stories with custom background knobs
  • Update parameters.info.story property to custom parameters.markdown property
  • Replace STORYBOOK_LIGHT_THEME usage, mainly in partition charts, with backgrounds default (e.g. backgrounds: { default: 'White' })

Theme and background color toggle

Leveraging the @storybook/addon-backgrounds and storybook-addon-themes we can now switch theme or background color for every story automatically (kinda -- we need to add useBaseTheme hook).

Screen.Recording.2021-07-26.at.04.27.44.PM.mp4

Note: this allows changing the background color independently of the theme which could look strange.

Futre Todos

Checklist

  • The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
  • The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
  • Whenever possible, please check if the closing issue is connected to a running GH project
  • Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@nickofthyme nickofthyme added the :build Build tools / dependencies label Jul 7, 2021
- create useBaseTheme to merge selected theme and backgroundColor
- set baseTheme on all stories via useBaseTheme hook
- Replace STORYBOOK_LIGHT_THEME with backgrounds: { default: 'White' }
- remove unnecessary story-chart class
- fix linting errors
Copy link
Collaborator Author

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Some comments to explain these changes.

.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
docs/0-Intro/1-Overview.mdx Show resolved Hide resolved
integration/server/index.ejs Show resolved Hide resolved
storybook/style.scss Outdated Show resolved Hide resolved
storybook/style.scss Show resolved Hide resolved
storybook/story_wrapper.tsx Show resolved Hide resolved
stories/shared.ts Show resolved Hide resolved
@nickofthyme nickofthyme marked this pull request as ready for review July 26, 2021 21:32
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

It looks great to me! Tested locally and works fine, I love the new theme switcher.
Great work nick!
just a tip: I kindly suggest to avoid upgrading eslint or prettier in a big PR to avoid a lot of changes that are not out of the scope of the PR

package.json Show resolved Hide resolved
"circular-dependency-plugin": "^5.2.2",
"sass": "~1.32.0",
"sass-loader": "^10.1.1",
"storybook-addon-themes": "git://github.com/nickofthyme/storybook-addon-themes.git",
Copy link
Member

Choose a reason for hiding this comment

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

are we going to move in the future those forks within the chart library or within the elastic github account?

Comment on lines +45 to +48
{ name: 'White', value: '#fff' },
// { name: 'White', value: euiLightVars.euiPageBackgroundColor },
{ name: 'Black', value: '#1D1E24' },
// { name: 'Black', value: euiDarkVars.euiPageBackgroundColor },
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 can keep our own, EUI has it's won chart theme and I don't want to be dependent on them right now

storybook/stories/annotations/lines/8_advanced_markers.tsx Outdated Show resolved Hide resolved
@@ -30,32 +33,38 @@ const MAX_CYCLES = 0;
let numCyclesDetected = 0;

module.exports = async ({ config }) => {
const FAST = Boolean(JSON.parse(process.env.VRT)) ?? false;
Copy link
Member

@markov00 markov00 Jul 27, 2021

Choose a reason for hiding this comment

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

Please remember that we don't execute VRT against storybook, but the full process is actually on a different server and configuration.
There are also multiple commands related to CI and VRT associated with storybook like the current start script:

VRT=true start-storybook -s ./public -p 9001 -c storybook --ci --no-version-updates

I think we can remove VRT and --ci

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

Tested locally LGTM! I like the theme picker instead of having to do boolean knobs with storybook. Also thank you for setting up the useBaseTheme() to use for future stories

import { mocks } from '../../packages/charts/src/mocks/hierarchical/index';
import { config } from '../../packages/charts/src/chart_types/partition_chart/layout/config';
import { ShapeTreeNode } from '../../packages/charts/src/chart_types/partition_chart/layout/types/viewmodel_types';
import { Chart, Datum, Partition, PartitionLayout } from '@elastic/charts';
Copy link
Contributor

Choose a reason for hiding this comment

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

file name could change to sunburST

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM! I ran it locally, and also onion sliced about 10% of the changed mocks

@nickofthyme
Copy link
Collaborator Author

Jenkins retest this please

@markov00
Copy link
Member

Jenkins retest this please

There is only a slow startup for the VRT server.
Locally it run in less then 100 seconds so it should be fine on CI

@nickofthyme nickofthyme merged commit 873f4e0 into elastic:master Jul 29, 2021
@nickofthyme nickofthyme deleted the update-storybook branch July 29, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:build Build tools / dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants