Add storybook #99

zhongyn · 2017-11-03T16:20:21Z

Summary

Setup storybook as doc site, migrate nav/layout/icon examples to storybook

npm install
npm run storybook
visit localhost:8081

terra-bot · 2017-11-03T16:26:31Z

	Warnings
⚠️	❗ Big PR. Consider breaking this into smaller PRs if applicaple

Generated by 🚫 dangerJS

mhemesath · 2017-11-04T15:21:13Z

I prefer that the stories live in the package of each component. We can then use story discovery to pull all the stories into storybook.

e.g https://github.com/cerner/terra-core/blob/storybook/.storybook/config.js

mhemesath · 2017-11-04T15:25:15Z

Can we update the the deployment to run storybook rather than site? I want to remove the site, but we can do that in a subsequent pull request when we switch everything to webdriver.io and storybook

bjankord · 2017-11-05T13:24:44Z

@mhemesath Storybook just added a feature to autoload all files with /.story.js$/ in their name to their v3.3.0 release branch. v3.3.0 has not yet been released yet though, there are a few other issues in that milestone.

zhongyn · 2017-11-07T16:23:10Z

@mhemesath putting the stories in the package of each component works for most components except those with translations. In this PR, all stories and story configs are inside the storybook folder and isolate from the outside.

root
- storybook
  - package.json
  - config.js
  - webpack.config.js
  - stories
- packages
- package.json

so it can install terra-consumer-nav and add terra-i18n-plugin in its own node_modules. terra-in8-plugin then aggregates the translations by searching the node_modules folder. i18n is required for component like NavHelp.

In the terra-badge example, the structure is:

root
- .storybook
  - config.js
- packages
  - terra-badge
    - stories
- package.json

That works. In the case of terra-alert with translations, it requires adding webpack.config.js with terra-i18n-plugin inside .storybook, and it will try to search the root level node_modules for translations folder. That means terra-alert should be added as devDependency in the root level. Not sure if that's appropriate. Any idea for handling this case?

zhongyn · 2017-11-07T16:25:13Z

yes, will update the deployment to run storybook after this PR

mhemesath · 2017-11-07T17:25:35Z

terra-i18n-plugin then aggregates the translations by searching the node_modules folder

That plugin is only needed for aggregation. Do we need aggregation with storybook? Why can't the component find the translations relative to its directory. If we do need i18n plugin, can the source folder be modified it uses to search for translations?

mhemesath · 2017-11-07T17:26:13Z

I am really adamant about having the stories of the components live in each package if possible.

zhongyn · 2017-11-07T20:02:22Z

That plugin is only needed for aggregation. Do we need aggregation with storybook?

yes, terra-consumer-nav depends on terra-i18n, and i18n loads translation files from the aggregation folder. i18n-plugin generates the aggregation. Without the aggregation folder webpack compile will fail. One solution is creating the aggregation manually with empty message. If so terra-i18n will work but no message is pulled in the component.

Why can't the component find the translations relative to its directory.

terra-i18n at the root level pulls in the translations. The component itself doesn't load the translations.

If we do need i18n plugin, can the source folder be modified it uses to search for translations?

i18n plugin only checks the node_modules for translation files. We can update it to take a custom path param so that it can search any custom folder

zhongyn · 2017-11-07T20:33:05Z

I created an issue in terra-core for extending i18n plugin with custom search path: cerner/terra-core#998

zhongyn · 2017-11-09T19:52:33Z

package.json

    "terra-i18n": "^1.4.0",
+    "terra-i18n-plugin": "file:../terra-core/packages/terra-i18n-plugin",


just for local dev, after this PR is merged we can update it with the new version

bjankord · 2017-11-29T18:20:46Z

package.json

    "test": "npm run jest && npm run nightwatch"
  },
  "devDependencies": {
+    "@storybook/addon-actions": "^3.2.14",


You may want to update to 3.3.0-alpha.4. Version 3.2 had issues with loading JSONP which is needed for requiring translations. This issue was fixed in v3.3.0 alphas. More info: storybookjs/storybook#1411

mhemesath · 2017-12-04T15:26:26Z

.storybook/preview-head.html

@@ -0,0 +1,3 @@
+<script>
+  document.documentElement.setAttribute("dir", "ltr");


you can probably just do this in your main decorator

addDecorator(story => <div dir="ltr"><Base locale="en-US">{story()}</Base></div>);

Terra might log a warning, but the attribute will still work even if its not on HTML tag

neat! will update

just found that it doesn't work for popup/modal rendered outside the Base tree. NavHelp looks like this after the change:

So we still need to put it in html tag

mhemesath · 2017-12-04T15:26:58Z

.storybook/webpack.config.js

+const rtl = require('postcss-rtl');
+
+const customProperties = CustomProperties({ preserve: true, warnings: false });
+customProperties.setVariables({


is this duplicated from our other webpack?

it's copied from terra-consumer-site webpack config. Probably it's not necessary, will remove it and test the code

alex-bezek temporarily deployed to terra-consumer-pr-99 November 3, 2017 16:20 Inactive

alex-bezek had a problem deploying to terra-consumer-pr-99 November 9, 2017 19:49 Failure

alex-bezek had a problem deploying to terra-consumer-pr-99 November 9, 2017 19:50 Failure

zhongyn commented Nov 9, 2017

View reviewed changes

YZ044636 added 4 commits November 29, 2017 09:35

Add storybook

f6989bc

Move stories to component src

d56d212

update package json

921c1cd

update storybook config and examples

388410a

zhongyn force-pushed the add-storybook branch from 656be3a to 388410a Compare November 29, 2017 17:16

alex-bezek temporarily deployed to terra-consumer-pr-99 November 29, 2017 17:16 Inactive

update package.json

0d49ad3

alex-bezek temporarily deployed to terra-consumer-pr-99 November 29, 2017 17:33 Inactive

update contributors

c40aea5

alex-bezek temporarily deployed to terra-consumer-pr-99 November 29, 2017 17:42 Inactive

bjankord reviewed Nov 29, 2017

View reviewed changes

update addon version

9dfc9b2

alex-bezek temporarily deployed to terra-consumer-pr-99 November 29, 2017 19:27 Inactive

zhongyn requested review from mhemesath, bjankord and alex-bezek December 1, 2017 15:21

mhemesath reviewed Dec 4, 2017

View reviewed changes

mhemesath approved these changes Dec 4, 2017

View reviewed changes

update webpack config

466e929

alex-bezek approved these changes Dec 4, 2017

View reviewed changes

alex-bezek deployed to terra-consumer-pr-99 December 4, 2017 17:14 View deployment

zhongyn merged commit b79bc25 into master Dec 4, 2017

zhongyn deleted the add-storybook branch December 4, 2017 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storybook #99

Add storybook #99

zhongyn commented Nov 3, 2017

terra-bot commented Nov 3, 2017 •

edited

Loading

mhemesath commented Nov 4, 2017 •

edited

Loading

mhemesath commented Nov 4, 2017

bjankord commented Nov 5, 2017

zhongyn commented Nov 7, 2017

zhongyn commented Nov 7, 2017

mhemesath commented Nov 7, 2017

mhemesath commented Nov 7, 2017

zhongyn commented Nov 7, 2017 •

edited

Loading

zhongyn commented Nov 7, 2017

zhongyn Nov 9, 2017 •

edited

Loading

bjankord Nov 29, 2017

mhemesath Dec 4, 2017

zhongyn Dec 4, 2017

zhongyn Dec 4, 2017

mhemesath Dec 4, 2017

zhongyn Dec 4, 2017

		"terra-i18n": "^1.4.0",
		"terra-i18n-plugin": "file:../terra-core/packages/terra-i18n-plugin",

		@@ -0,0 +1,3 @@
		<script>
		document.documentElement.setAttribute("dir", "ltr");

Add storybook #99

Add storybook #99

Conversation

zhongyn commented Nov 3, 2017

Summary

terra-bot commented Nov 3, 2017 • edited Loading

mhemesath commented Nov 4, 2017 • edited Loading

mhemesath commented Nov 4, 2017

bjankord commented Nov 5, 2017

zhongyn commented Nov 7, 2017

zhongyn commented Nov 7, 2017

mhemesath commented Nov 7, 2017

mhemesath commented Nov 7, 2017

zhongyn commented Nov 7, 2017 • edited Loading

zhongyn commented Nov 7, 2017

zhongyn Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

bjankord Nov 29, 2017

Choose a reason for hiding this comment

mhemesath Dec 4, 2017

Choose a reason for hiding this comment

zhongyn Dec 4, 2017

Choose a reason for hiding this comment

zhongyn Dec 4, 2017

Choose a reason for hiding this comment

mhemesath Dec 4, 2017

Choose a reason for hiding this comment

zhongyn Dec 4, 2017

Choose a reason for hiding this comment

terra-bot commented Nov 3, 2017 •

edited

Loading

mhemesath commented Nov 4, 2017 •

edited

Loading

zhongyn commented Nov 7, 2017 •

edited

Loading

zhongyn Nov 9, 2017 •

edited

Loading