Add a storybook for the ui package #2504

tmeasday · 2017-12-18T06:05:28Z

Issue:

No storybook exists for storybook's UI itself 😮

What I did

Added stories for the lib/ui package

How to test

Run yarn storybook from the top level.

I'm not sure if this is the best approach. We could also add a storybook inside lib/ui but we'd have a circular dependency issue.

Note as well that I added the stories for the lib/components package to this storybook; I note they've also been added to the cra-kitchen-sink app -- perhaps this is a better approach?

Let me know what you think.

Is this testable with jest or storyshots?

Potentially we could use storyshots?

Does this need a new example in the kitchen sink apps?

NO

Does this need an update to the documentation?

NO

ndelangen · 2017-12-18T10:45:26Z

.storybook/addons.js

@@ -0,0 +1,3 @@
+/* eslint-disable import/no-extraneous-dependencies, import/no-unresolved, import/extensions */


Can we not use a /examples/<any> here? Do we really have to add something to the root of the project?

I would be happy with an examples/official or something, where we have a storybook to demo storybook's UI components.

I don't have any problem with that. The only thing is potentially wanting to combine all react-based storybooks in a single storybook (components, ui, cra-vanilla).

[This works better for chromatic for instance. I'm not sure it helps much else -- maybe storyshots?]

Maybe examples/offical-storybook or something?

Or examples/meta[-storybook]?

examples/offical-storybook would get my vote

ndelangen · 2017-12-18T10:49:59Z

Lot's of love here, This will be fantastic to have!

Just hoping we can move the files away from the project root, since we're already crowding the project root folder a lot.

Hypnosphi · 2017-12-18T12:00:56Z

Note as well that I added the stories for the lib/components package to this storybook; I note they've also been added to the cra-kitchen-sink app -- perhaps this is a better approach?

In fact, I originally put them to lib/components, but @ndelangen asked me to move them to cra-kitchen-sink.
And now I actually feel very convenient to have all the examples in a single storybook instance. So maybe we should move the lib/ui stories there as well

tmeasday · 2017-12-18T12:04:50Z

I actually kind of feel like a storybook/ directory at the top level would be kind of a cool thing to have?

ndelangen · 2017-12-18T12:55:32Z

I don't that will convey meaning to anyone just browsing through the code, where-as a folder /examples/official-storybook would.

Of course this is really subjective, Personally I feel fairly strongly, that grouping the examples together is the right approach, but I can change my mind, if 3/4 others vote in favour of your proposal @tmeasday.

To me, it's just another example of storybook that we use internally to develop and test our work on.

Hypnosphi · 2017-12-18T22:30:42Z

I actually kind of feel like a storybook/ directory at the top level would be kind of a cool thing to have?

Maybe, but only if we move all the other sample stories there

I'm strongly inclined into having all the React examples in one place. Currently it's example-kitchen-sink. That way, the workflow is pretty much the same for any of the changes made or reviewed.

One of the cool parts about it is that we deploy kitchen-sink apps to netlify for each PR, so there's a way to test things without actually cheking out the code locally

tmeasday · 2017-12-19T02:58:47Z

I'm not very opinionated about this, but I would say as a new developer I would probably not expect to find the stories about the storybook UI in the "kitchen sink" example.

I think we all agree that all the react-based stories (ui, components, various addons, other stuff in KS) should be in the one storybook. My vote would be examples/official-storybook. Then we can make KS simple again (like the other KSs) and move any tricky stuff like netlify over to official-storybook.

Hypnosphi · 2017-12-19T12:52:01Z

Sounds good

ndelangen · 2017-12-19T23:31:18Z

Let's do it!

tmeasday · 2017-12-20T00:15:23Z

On the case.

ndelangen · 2017-12-20T11:13:55Z

That chromatic is fantastic @tmeasday !

Will Chromatic also show up as a github status?

tmeasday · 2017-12-21T00:05:53Z

@ndelangen sure does! Check out the chromatic PR: #2505

I'll take the lack of negative comments as a positive and I'll get the tests passing on this branch.

tmeasday · 2017-12-21T03:04:26Z

I'm getting this error from storyshots:

[187:0x36cbe80]   105747 ms: Mark-sweep 1400.4 (1454.0) -> 1400.4 (1454.0) MB, 1403.1 / 0.0 ms  allocation failure GC in old space requested
[187:0x36cbe80]   107011 ms: Mark-sweep 1400.4 (1454.0) -> 1400.4 (1452.5) MB, 1262.8 / 0.0 ms  last resort GC in old space requested
[187:0x36cbe80]   108309 ms: Mark-sweep 1400.4 (1452.5) -> 1400.4 (1452.5) MB, 1297.2 / 0.0 ms  last resort GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x3719efba5ee1 <JSObject>
    1: read [/tmp/storybook/node_modules/react-dom/cjs/react-dom-server.node.development.js:~2206] [pc=0x23f6ad3e54b2](this=0x2bc5637e721 <ReactDOMServerRenderer map = 0x51a975c0841>,bytes=0x3795986829a9 <Number inf>)
    3: renderToStaticMarkup [/tmp/storybook/node_modules/react-dom/cjs/react-dom-server.node.development.js:2512] [bytecode=0x3b8b963a9489 offset=31](this=0x1665b6d8da41 <Object ma...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [node]
 2: 0x121a7ac [node]
 3: v8::Utils::ReportOOMFailure(char const*, bool) [node]
 4: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [node]
 5: v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [node]
 6: v8::internal::Runtime_AllocateInTargetSpace(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 7: 0x23f6ace0463d

Have you guys seen this before?

codecov · 2017-12-21T06:52:35Z

Codecov Report

Merging #2504 into release/3.3 will increase coverage by 0.73%.
The diff coverage is 83.14%.

@@               Coverage Diff               @@
##           release/3.3    #2504      +/-   ##
===============================================
+ Coverage        19.45%   20.18%   +0.73%     
===============================================
  Files              387      398      +11     
  Lines             8733     8838     +105     
  Branches           940      961      +21     
===============================================
+ Hits              1699     1784      +85     
+ Misses            6303     6300       -3     
- Partials           731      754      +23

Impacted Files	Coverage Δ
...b/components/src/navigation/routed_link.stories.js	`100% <100%> (ø)`
.../ui/src/modules/ui/components/menu_item.stories.js	`100% <100%> (ø)`
...ules/ui/components/stories_panel/header.stories.js	`100% <100%> (ø)`
...ui/components/stories_panel/text_filter.stories.js	`100% <100%> (ø)`
lib/components/src/navigation/menu_link.stories.js	`100% <100%> (ø)`
...modules/ui/components/addon_panel/index.stories.js	`100% <100%> (ø)`
.../src/modules/ui/components/layout/index.stories.js	`100% <100%> (ø)`
lib/ui/src/libs/withLifecycleDecorator.js	`17.14% <37.5%> (ø)`
...ui/src/modules/ui/components/search_box.stories.js	`62.5% <62.5%> (ø)`
...dules/ui/components/stories_panel/index.stories.js	`90.9% <90.9%> (ø)`
... and 91 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 c97eaf9...1a0e285. Read the comment docs.

ndelangen · 2017-12-21T06:54:29Z

Definitely the right the direction @tmeasday, fantastic work!

Don't compile (i.e. copy) them w/ babel

tmeasday · 2017-12-21T09:10:33Z

The segfault was something to do with react-modal. Should probably figure it out but I'll just disable that test for storyshots for now.

Fun times! babel/babel#6226 We don't want to copy `.storyshots` files even though babel doesn't think they are JS

tmeasday · 2017-12-21T22:55:58Z

@ndelangen shall I merge this?

ndelangen · 2017-12-22T00:21:04Z

Yes merge when ready, after, we should add a netlify site for the newly created example!

Hypnosphi · 2017-12-22T08:41:03Z

Why UI and components stories are not part of official-storybook?

tmeasday · 2017-12-22T10:24:08Z

How do you mean? Aren’t they?

…

On Fri, 22 Dec 2017 at 7:41 pm, Filipp Riabchun ***@***.***> wrote: Why UI and components stories are not part of "official"? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIFyp1XP-nkfQK8vPXWvyHGqivrYb6Qks5tC2stgaJpZM4RFAuq> .

Hypnosphi · 2017-12-22T10:29:15Z

Oh, now I see that they are. But their sources are in packages. We probably should use a single approach then. That is, either move ui and components stories sources to examples/official-storybook directory, or move all the addons' stories to corresponding packages.

CC: @ndelangen

ndelangen · 2017-12-22T12:21:16Z

Move all stories into the examples please.

Our packages shouldn't contain stories I think. Please let me know if you disagree on that.

I think co-locating the stories makes a bit more sense in this case. Reason for this is that we want to use the example/* as a way of showing off how to write stories.

tmeasday · 2017-12-22T22:30:43Z

Hmm, I can see the argument for this but it'd be a bit weird -- in particular the lib/ui stories mirror the .test.js files in that project (in the future it would be good to consolidate those further). When someone works on that project, wouldn't it be annoying to have to update a story file in a completely different location?

Maybe we should use a symlink or something?

ndelangen · 2017-12-23T00:24:44Z

🤔

tmeasday · 2017-12-23T00:38:52Z

I'm stumped too ;)

tmeasday · 2017-12-26T23:06:10Z

@ndelangen still willing to make whichever changes make sense here, I'm just not sure what's best

tmeasday added 5 commits December 18, 2017 15:05

Added first story as POC

e77a198

Added stories for @storybook/components

e625eb8

Added stories for stories tree

2ed0698

Added remaining stories for stories_panel

51361a8

Added stories for remaining components

e50ceaa

tmeasday requested review from ndelangen, shilman and Hypnosphi December 18, 2017 06:05

tmeasday changed the title ~~Tmeasday/add stories~~ Add a storybook for the ui package Dec 18, 2017

tmeasday mentioned this pull request Dec 18, 2017

Add chromatic visual tests #2505

Merged

ndelangen requested changes Dec 18, 2017

View reviewed changes

Merge branch 'release/3.3' into tmeasday/add-stories

c4bc58b

ndelangen added maintenance User-facing maintenance tasks ui labels Dec 18, 2017

ndelangen assigned tmeasday Dec 18, 2017

Merge branch 'release/3.3' into tmeasday/add-stories

04dc8e0

tmeasday added 5 commits December 20, 2017 15:16

Use linked package for ui storybook dev dependencies

1de6949

Use correct storybook version in components

f3d72ad

Created examples/official-storybook with ui/components/addon stories

fda8975

Move more complex component stories over

ae60bb8

Use all addons in official storybook

92760d3

Merge branch 'release/3.3' into tmeasday/add-stories

f2dbe6d

Working on moving most storyshots tests over to official-example

3b1d353

ndelangen approved these changes Dec 21, 2017

View reviewed changes

Merge branch 'release/3.3' into tmeasday/add-stories

432e2db

tmeasday added 2 commits December 21, 2017 20:09

Disable ui/SearchBox story in storyshots

3b8be68

Add snapshots for new stories.

913324e

Don't compile (i.e. copy) them w/ babel

tmeasday added 2 commits December 21, 2017 20:38

Move --copy-files *before* --ignore for babel

579cde7

Fun times! babel/babel#6226 We don't want to copy `.storyshots` files even though babel doesn't think they are JS

Ignore __snapshots__ when building

e4be9f5

tmeasday force-pushed the tmeasday/add-stories branch from ae4ed52 to e4be9f5 Compare December 21, 2017 21:57

Merge branch 'release/3.3' into tmeasday/add-stories

8823299

Merge branch 'release/3.3' into tmeasday/add-stories

1a0e285

tmeasday merged commit cbe6560 into release/3.3 Dec 22, 2017

tmeasday deleted the tmeasday/add-stories branch December 22, 2017 00:34

Hypnosphi mentioned this pull request Jan 2, 2018

Add ability to use image snapshots to addon-storyshots #2413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a storybook for the ui package #2504

Add a storybook for the ui package #2504

tmeasday commented Dec 18, 2017 •

edited

Loading

ndelangen Dec 18, 2017

ndelangen Dec 18, 2017

tmeasday Dec 18, 2017

tmeasday Dec 18, 2017

ndelangen Dec 18, 2017

ndelangen commented Dec 18, 2017

Hypnosphi commented Dec 18, 2017

tmeasday commented Dec 18, 2017

ndelangen commented Dec 18, 2017 •

edited

Loading

Hypnosphi commented Dec 18, 2017 •

edited

Loading

tmeasday commented Dec 19, 2017

Hypnosphi commented Dec 19, 2017

ndelangen commented Dec 19, 2017

tmeasday commented Dec 20, 2017

ndelangen commented Dec 20, 2017

tmeasday commented Dec 21, 2017

tmeasday commented Dec 21, 2017

codecov bot commented Dec 21, 2017 •

edited

Loading

ndelangen commented Dec 21, 2017

tmeasday commented Dec 21, 2017

tmeasday commented Dec 21, 2017

ndelangen commented Dec 22, 2017

Hypnosphi commented Dec 22, 2017 •

edited

Loading

tmeasday commented Dec 22, 2017 via email

Hypnosphi commented Dec 22, 2017

ndelangen commented Dec 22, 2017

tmeasday commented Dec 22, 2017

ndelangen commented Dec 23, 2017

tmeasday commented Dec 23, 2017

tmeasday commented Dec 26, 2017

		@@ -0,0 +1,3 @@
		/* eslint-disable import/no-extraneous-dependencies, import/no-unresolved, import/extensions */

Add a storybook for the ui package #2504

Add a storybook for the ui package #2504

Conversation

tmeasday commented Dec 18, 2017 • edited Loading

What I did

How to test

Is this testable with jest or storyshots?

Does this need a new example in the kitchen sink apps?

Does this need an update to the documentation?

ndelangen Dec 18, 2017

Choose a reason for hiding this comment

ndelangen Dec 18, 2017

Choose a reason for hiding this comment

tmeasday Dec 18, 2017

Choose a reason for hiding this comment

tmeasday Dec 18, 2017

Choose a reason for hiding this comment

ndelangen Dec 18, 2017

Choose a reason for hiding this comment

ndelangen commented Dec 18, 2017

Hypnosphi commented Dec 18, 2017

tmeasday commented Dec 18, 2017

ndelangen commented Dec 18, 2017 • edited Loading

Hypnosphi commented Dec 18, 2017 • edited Loading

tmeasday commented Dec 19, 2017

Hypnosphi commented Dec 19, 2017

ndelangen commented Dec 19, 2017

tmeasday commented Dec 20, 2017

ndelangen commented Dec 20, 2017

tmeasday commented Dec 21, 2017

tmeasday commented Dec 21, 2017

codecov bot commented Dec 21, 2017 • edited Loading

Codecov Report

ndelangen commented Dec 21, 2017

tmeasday commented Dec 21, 2017

tmeasday commented Dec 21, 2017

ndelangen commented Dec 22, 2017

Hypnosphi commented Dec 22, 2017 • edited Loading

tmeasday commented Dec 22, 2017 via email

Hypnosphi commented Dec 22, 2017

ndelangen commented Dec 22, 2017

tmeasday commented Dec 22, 2017

ndelangen commented Dec 23, 2017

tmeasday commented Dec 23, 2017

tmeasday commented Dec 26, 2017

tmeasday commented Dec 18, 2017 •

edited

Loading

ndelangen commented Dec 18, 2017 •

edited

Loading

Hypnosphi commented Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 21, 2017 •

edited

Loading

Hypnosphi commented Dec 22, 2017 •

edited

Loading