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.

Sign up for GitHub

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

Refactored storybook component library #1266

Merged
merged 8 commits into from
Jun 16, 2017
+195 −1,544
Merged

Refactored storybook component library #1266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3fdd1a1
ADD package
ndelangen Jun 11, 2017
08f3f83
WIP
ndelangen Jun 11, 2017
294923b
Use new common demo-components
ndelangen Jun 12, 2017
abac2be
Merge branch 'master' into add-lib-components
ndelangen Jun 12, 2017
b7a9110
Merge branch 'master' into add-lib-components
ndelangen Jun 14, 2017
59d4cc2
Merge branch 'master' into add-lib-components
ndelangen Jun 16, 2017
436767d
test
ndelangen Jun 16, 2017
0be8829
Merge branch 'master' into add-lib-components
ndelangen Jun 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ build
coverage
node_modules
**/example/**
**/demo/**
app/**/demo/**
docs/public

*.bundle.js
Expand Down
4 changes: 4 additions & 0 deletions addons/storyshots/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
"read-pkg-up": "^2.0.0"
},
"devDependencies": {
"@storybook/addons": "^3.0.0",
"@storybook/channels": "^3.0.0",
"@storybook/components": "^3.0.0",
"babel-cli": "^6.24.1",
"babel-plugin-transform-runtime": "^6.23.0",
"babel-preset-es2015": "^6.24.1",
Expand All @@ -30,6 +33,7 @@
"peerDependencies": {
"@storybook/addons": "^3.0.0",
"@storybook/channels": "^3.0.0",
"@storybook/components": "^3.0.0",
"babel-core": "^6.24.1",
"react": "*",
"react-test-renderer": "*"
Expand Down
28 changes: 0 additions & 28 deletions addons/storyshots/stories/directly_required/Button.js
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion addons/storyshots/stories/directly_required/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
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';

storiesOf('Another Button', module)
.add('with text', () => <Button onClick={action('clicked')}>Hello Button</Button>)
Expand Down
2 changes: 1 addition & 1 deletion addons/storyshots/stories/required_with_context/Button.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not?

Copy link
Member

@tmeasday tmeasday Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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:

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

  2. 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?

  3. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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


storiesOf('Button', module)
.add('with text', () => <Button onClick={action('clicked')}>Hello Button</Button>)
Expand Down
127 changes: 0 additions & 127 deletions addons/storyshots/stories/required_with_context/Welcome.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion addons/storyshots/stories/required_with_context/Welcome.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import React from 'react';
import { storiesOf } from '@storybook/react'; // eslint-disable-line
import { linkTo } from '@storybook/addon-links'; // eslint-disable-line

import Welcome from './Welcome';
import Welcome from '@storybook/components/dist/demo/Welcome';

storiesOf('Welcome', module).add('to Storybook', () => <Welcome showApp={linkTo('Button')} />);
1 change: 1 addition & 0 deletions app/react-native/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@storybook/addon-links": "^3.1.2",
"@storybook/addons": "^3.1.2",
"@storybook/channel-websocket": "^3.1.2",
"@storybook/components": "^3.0.0",
"@storybook/ui": "^3.1.3",
"autoprefixer": "^7.0.1",
"babel-core": "^6.24.1",
Expand Down
15 changes: 0 additions & 15 deletions app/react/demo/.gitignore
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions app/react/demo/.storybook/addons.js
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions app/react/demo/.storybook/config.js
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions app/react/demo/README.md
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions app/react/demo/package.json
View file
Open in desktop

This file was deleted.

Binary file removed app/react/demo/public/favicon.ico
View file
Open in desktop
Binary file not shown.
31 changes: 0 additions & 31 deletions app/react/demo/public/index.html
View file
Open in desktop

This file was deleted.

24 changes: 0 additions & 24 deletions app/react/demo/src/App.css
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions app/react/demo/src/App.js
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions app/react/demo/src/App.test.js
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions app/react/demo/src/index.css
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions app/react/demo/src/index.js
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions app/react/demo/src/logo.svg
View file
Open in desktop

This file was deleted.

Loading