Refactored storybook component library #1266

ndelangen · 2017-06-12T17:39:33Z

Issue:

-demo components are duplicated-
-we need a method to share UI components for core apps and addons-
-we need theme-ability for our apps- Add theming support #428 Custom storybook UI #313

What I did

I added a lib/package that will house all styled UI components.

How to test

npm run test
view examples/cra-storybook's Welcome and Button component
use the cli to generate

tmeasday

A couple of small things, otherwise 👍

tmeasday · 2017-06-14T05:38:19Z

addons/storyshots/stories/directly_required/Button.js

@@ -1,28 +0,0 @@
-import PropTypes from 'prop-types';


In theory this file is testing different ways of requiring stories in storyshots.

In practice we don't exercise this test right now. I'm not sure how to do it :/

In any case, I'm not sure we should remove it yet?

There's no difference in requiring it from a local file or from a npm package?

I'd like to remove duplication..

@ndelangen some of the files in the storyshots/stories are required directly (require('path/to/file')) in the .storybook/config.js, where as some others are required via require.context. I guess the idea is to test both methods.

I think your changes removes that and just requires everything directly.

As we aren't testing everything right now I wouldn't block on this, but I wanted to point it out.

tmeasday · 2017-06-14T05:38:54Z

addons/storyshots/stories/required_with_context/Button.stories.js

@@ -3,7 +3,7 @@ import React from 'react';
 import { storiesOf } from '@storybook/react'; // eslint-disable-line
 import { action } from '@storybook/addon-actions'; // eslint-disable-line

-import Button from './Button';
+import Button from '@storybook/components/dist/demo/Button';


Can we export Button directory from the package? Importing from a sub-path doesn't seem kosher to me.

@tmeasday @ndelangen my understanding (possibly wrong) was that demo is just one type of component, and that we are going to move all the storybook components into here. If that's the case, I think importing from a sub-path makes sense.

My understanding is that it's generally not a great idea to ask users to require from files inside your package. It's obviously not universal not to do it (e.g. react-test-renderer/shallow), but I think you'll notice most packages are moving away from it. The main reason is it is brittle:

You lose the flexibility to re-arrange how your files are organised in your package--certain files have to remain no matter what.

You expose details of your build process (cf /dist/ being in the path). This is a bit ugly and restrictive again--e.g. what happens in the future when we don't need babel any more and dist is redundant? Or if lerna has some great new technique that favours a different naming of output files?

Also, tooling understands the module entry point (package.json#main), whereas other entry points are just not going to be picked up. This also means new devs don't have any way to know which of our files users may be importing, exacerbating fix sidebar - content float issue #1.

It doesn't seem that hard to export everything from a common entry file to me.

import DemoButton from '@storybook/components';

OTOH, Storybook already asks users to import from paths a bit so if you want to do it, fine.

@tmeasday I'll think about it. Exposing all components as 1 module would mean we should allow tree shaking, because otherwise there's the potential for package bloat.

In fact maybe we should open an issue for exposing the es-modules code for all packages https://github.com/rollup/rollup/wiki/pkg.module

ndelangen · 2017-06-14T15:21:26Z

Thanks for the review @tmeasday, I can't really find anything actionable in your first comment, and would like some additional info on the second one.

usulpro · 2017-06-15T00:55:10Z

I fully support the idea of having the only package with demo components and use it for all demos!

But getstorybook a bit another case. We add this files to the project code to let users play with them as the simplest example. I guess users remove Button.js and 'Welcome.js' from theirs storybooks later. So removing sources and adding packages possible could confuse?

We can copy these files from package directly to the project
Here is PR for that against this branch #1283
still one source of code but instead of requiring from the package it copies these files

tmeasday · 2017-06-15T01:06:12Z

@usulpro I'm not sure, I always found the fact that storybook puts down component files kind of weird and confusing. I feel like having defined components in your stories folder would be a source of confusion for people "is this what I'm supposed to do? I thought storybook was for testing my components?".

I guess adding a dependency on @storybook/components which eventually will be unused (directly) by the app isn't ideal but (a) it's a devDependency, and (b) the storybook packages themselves will depend on it anyway, so it would have been installed anyway.

# Conflicts: # examples/cra-kitchen-sink/src/stories/Button.js # examples/cra-kitchen-sink/src/stories/Welcome.js

codecov · 2017-06-16T06:50:17Z

Codecov Report

Merging #1266 into master will increase coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1266      +/-   ##
==========================================
+ Coverage   13.71%   13.76%   +0.05%     
==========================================
  Files         207      201       -6     
  Lines        4646     4620      -26     
  Branches      517      573      +56     
==========================================
- Hits          637      636       -1     
+ Misses       3568     3493      -75     
- Partials      441      491      +50

Impacted Files	Coverage Δ
lib/components/src/demo/Welcome.js	`0% <0%> (ø)`
lib/components/src/demo/Button.js	`0% <0%> (ø)`
...ts/stories/required_with_context/Button.stories.js	`0% <0%> (ø)`	⬆️
...dons/storyshots/stories/directly_required/index.js	`0% <0%> (ø)`	⬆️
...s/stories/required_with_context/Welcome.stories.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/client_api.js	`39.28% <0%> (ø)`	⬆️
app/react/src/client/preview/reducer.js	`0% <0%> (ø)`	⬆️
.../src/manager/containers/CommentsPanel/dataStore.js	`34.78% <0%> (ø)`	⬆️
addons/knobs/src/components/WrapStory.js	`12% <0%> (ø)`	⬆️
addons/info/src/components/Props.js	`0% <0%> (ø)`	⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b244f99...0be8829. Read the comment docs.

ndelangen · 2017-06-16T11:31:52Z

@tmeasday I'd really like to get this merged.
This is a very important stepping stone for storybook IMHO.

I don't think the usage of deep file-requires is a too big of a deal to hold of merger.

shilman

¯_(ツ)_/¯

The deeplinks are an ok option for now, we will revisit this later

ndelangen added 3 commits June 11, 2017 22:58

ADD package

3fdd1a1

WIP

08f3f83

Use new common demo-components

294923b

ndelangen added addon: storyshots cli feature request maintenance User-facing maintenance tasks ui labels Jun 12, 2017

ndelangen self-assigned this Jun 12, 2017

ndelangen requested review from tmeasday and shilman June 12, 2017 17:39

ndelangen added the in progress label Jun 12, 2017

Merge branch 'master' into add-lib-components

abac2be

tmeasday previously requested changes Jun 14, 2017

View reviewed changes

Merge branch 'master' into add-lib-components

b7a9110

usulpro mentioned this pull request Jun 15, 2017

Generate template from lib components #1283

Closed

Merge branch 'master' into add-lib-components

59d4cc2

# Conflicts: # examples/cra-kitchen-sink/src/stories/Button.js # examples/cra-kitchen-sink/src/stories/Welcome.js

ndelangen force-pushed the add-lib-components branch 3 times, most recently from 44a8f80 to f15a548 Compare June 16, 2017 09:26

test

436767d

ndelangen force-pushed the add-lib-components branch from f15a548 to 436767d Compare June 16, 2017 09:29

Merge branch 'master' into add-lib-components

0be8829

shilman approved these changes Jun 16, 2017

View reviewed changes

ndelangen merged commit 40858bf into master Jun 16, 2017

ndelangen deleted the add-lib-components branch June 16, 2017 12:25

ndelangen removed the in progress label Jun 16, 2017

shilman removed the feature request label Jun 17, 2017

shilman changed the title ~~Add lib components~~ Refactored storybook component library Jun 17, 2017

This was referenced Jun 17, 2017

Fix bug in storybook common components refactor #1302

Closed

Import demo components from @storybook/react #1303

Merged

usulpro mentioned this pull request Jun 26, 2017

Add theming support #428

Closed

dependencies bot mentioned this pull request Nov 11, 2017

Update 2 dependencies from npm #2290

Merged

dependabot-preview bot mentioned this pull request Feb 5, 2019

Bump eslint-plugin-import from 2.15.0 to 2.16.0 #5480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored storybook component library #1266

Refactored storybook component library #1266

ndelangen commented Jun 12, 2017 •

edited

Loading

tmeasday left a comment

tmeasday Jun 14, 2017

ndelangen Jun 14, 2017

tmeasday Jun 15, 2017

tmeasday Jun 14, 2017

shilman Jun 14, 2017

ndelangen Jun 14, 2017

tmeasday Jun 15, 2017 •

edited

Loading

ndelangen Jun 16, 2017

ndelangen commented Jun 14, 2017

usulpro commented Jun 15, 2017 •

edited

Loading

tmeasday commented Jun 15, 2017 •

edited

Loading

codecov bot commented Jun 16, 2017 •

edited

Loading

ndelangen commented Jun 16, 2017

shilman left a comment

Refactored storybook component library #1266

Refactored storybook component library #1266

Conversation

ndelangen commented Jun 12, 2017 • edited Loading

What I did

How to test

tmeasday left a comment

Choose a reason for hiding this comment

tmeasday Jun 14, 2017

Choose a reason for hiding this comment

ndelangen Jun 14, 2017

Choose a reason for hiding this comment

tmeasday Jun 15, 2017

Choose a reason for hiding this comment

tmeasday Jun 14, 2017

Choose a reason for hiding this comment

shilman Jun 14, 2017

Choose a reason for hiding this comment

ndelangen Jun 14, 2017

Choose a reason for hiding this comment

tmeasday Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

ndelangen Jun 16, 2017

Choose a reason for hiding this comment

ndelangen commented Jun 14, 2017

usulpro commented Jun 15, 2017 • edited Loading

tmeasday commented Jun 15, 2017 • edited Loading

codecov bot commented Jun 16, 2017 • edited Loading

Codecov Report

ndelangen commented Jun 16, 2017

shilman left a comment

Choose a reason for hiding this comment

ndelangen commented Jun 12, 2017 •

edited

Loading

tmeasday Jun 15, 2017 •

edited

Loading

usulpro commented Jun 15, 2017 •

edited

Loading

tmeasday commented Jun 15, 2017 •

edited

Loading

codecov bot commented Jun 16, 2017 •

edited

Loading