Use parameters for info addon #3697

Keraito · 2018-06-01T14:00:53Z

Last of #3625

On top of implementing the new parameterized way of using addons for info, I also added made changes regarding how the options and parameters were processed with respect to each other, #3625 (comment). Before, the addon parameters would be the ones that were the latest set (global -> local -> story), replacing the previous ones entirely.

Now, it only replaces the addon properties per key, #3625 (comment). I don't think my solution in lib/core/src/client/preview/client_api.js is the cleanest one, so if anyone else has a better idea, happy to hear! I tried some stuff with reduce but that only made more of a mess.

I applied similar logic to the handling of options and parameters.

Core tests

2 snapshot tests failed in 1 test suite, both output:

    - Snapshot
    + Received
    
...

    - <Component>
    + <deprecated>

This seems unrelated to my changes, or am I mistaken?

tmeasday

Some comments to remove the old API a little more.

I'm not sure if info is a special case because it needs to be the first addon?

tmeasday · 2018-06-02T00:11:26Z

addons/info/README.md


 It is important to declare this decorator as **the first decorator**, otherwise it won't work well.

 ```js
-addDecorator((story, context) => withInfo('common info')(story)(context));
+addDecorator(withInfo);


I think it is worth indicating this should be done in .storybook/config.js

tmeasday · 2018-06-02T00:13:35Z

addons/info/README.md

+  );
+```
+
+
 ## Global options


Let's deprecate this and the usage as a HoC ("Basic usage" and "Usage with options") -- so let's remove it from the docs.

@tmeasday on a related note, do you think we should have a Deprecated section in storybook-official so we still get story snapshots for deprecated usage, but they are separate from the recommended usage examples?

@tmeasday Just so we're on the same line: you want to deprecate the setDefaults way of setting defaults (and shift that to the addDecorator in the config.js with either options or parameters) and remove all the docs involving this + the HoC?

Related to this, should I then rewrite all the existing info stories in official-storybook to use the parameterized way?

Sorry guys, missed this!

should have a Deprecated section in storybook-official so we still get story snapshots for deprecated usage, but they are separate from the recommended usage examples?

Probably a good idea @shilman -- perhaps a small refactor of the stories after we get the last few PRs merged is in order.

Just so we're on the same line: you want to deprecate the setDefaults way of setting defaults (and shift that to the addDecorator in the config.js with either options or parameters) and remove all the docs involving this + the HoC?

Yeah that was what I was thinking, do you agree? Definitely get rid of the HoC; and setDefaults seems redundant now, right?

Related to this, should I then rewrite all the existing info stories in official-storybook to use the parameterized way?

What I've been doing is leaving the existing ones as "tests" for the deprecated functionality (we can remove them when actually delete that functionality). Then making sure there are stories to cover all the new ways of doing it! You don't need to re-test that parameter inheritance works though (although maybe adding a the story in core.stories.js to test your change to the way it works?), just add stories that make sense.

I hope I'm making sense here 😬

tmeasday · 2018-06-02T00:14:22Z

addons/info/src/index.js

+export const withInfo = makeDecorator({
+  name: 'withInfo',
+  parameterName: 'info',
+  wrapper: (getStory, context, { options, parameters }) => {


Do we want to skip the addon if they don't set options or parameters? Either by passing skipIfNoOptionsOrParameters: true or manually doing it here..

Otherwise (or maybe anyway) we should document setting { info: { disable: true } }

Def agree with the disable: true part, will do. I'm not so sure about the skipIfNoOptionsOrParameters part though. I assume the both of them do the same thing, namely disable the addon entirely. But info is slightly different from the others addons imo that it does something even without configuration: display Proptypes, the component, etc.

Example: https://github.com/storybooks/storybook/pull/3697/files#diff-c915ecbefba3a2734ebd93eb258431dbR353 This is basically an parameters- and options-less story, besides some minor options (header styling) and parameters (inline and manual text). The configuration don't add a lot, but the addon itself adds a lot of information stand-alone already.

So I would say just only documenting the disable flag should be the only way

Ok, makes sense. Thanks for thinking about that.

tmeasday · 2018-06-02T00:21:09Z

lib/core/src/client/preview/client_api.js

+        if (params) {
+          Object.keys(params).forEach(key => {
+            if (typeof params[key] === 'object') {
+              allParam[key] = Object.assign({ ...allParam[key] }, params[key]);


Maybe just { ...allParam[key], ...params[key] }? Kind of odd to use Object.assign and object spread ;)

tmeasday · 2018-06-02T00:21:37Z

Look great btw! Thanks @Keraito

codecov · 2018-06-02T20:42:56Z

Codecov Report

Merging #3697 into master will increase coverage by 51.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #3697       +/-   ##
==========================================
+ Coverage   41.36%   92.5%   +51.13%     
==========================================
  Files         459       6      -453     
  Lines        5087      40     -5047     
  Branches      856       2      -854     
==========================================
- Hits         2104      37     -2067     
+ Misses       2473       2     -2471     
+ Partials      510       1      -509

Impacted Files	Coverage Δ
addons/viewport/preview.js
addons/info/src/components/types/ArrayOf.js
app/marko/src/server/index.js
app/angular/options.js
app/react/src/client/preview/element_check.js
lib/ui/src/modules/ui/configs/init_panels.js
addons/info/src/components/types/PropertyLabel.js
addons/links/src/manager.js
addons/graphql/src/preview.js
app/angular/src/server/options.js
... and 441 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 dfb0347...3cc1566. Read the comment docs.

Keraito · 2018-06-02T22:10:22Z

I processed most of the feedback. Waiting on a reply on #3697 (comment), then i will finish everything up

Keraito · 2018-06-04T08:48:44Z

Oops I totally referenced the wrong comment in my last message, I meant this one #3697 (comment) (about deprecating the previous usage and what to change exactly). 🙈sorry @tmeasday could you take a look again?

Keraito · 2018-06-05T14:12:36Z

addons/info/src/index.js

@@ -98,5 +99,8 @@ export const withInfo = makeDecorator({
 export { Story };

 export function setDefaults(newDefaults) {
-  return Object.assign(defaultOptions, newDefaults);
+  return deprecate(


I don't feel like this is how I should've implemented the depreciation, but only this worked for me for some reason.

@Keraito you tried export const setDefaults = deprecate(newDefaults => Object.assign(...), 'setDefaults is ...')

I mean what you have is fine anyway ;)

Keraito · 2018-06-05T14:45:42Z

Should be all done now, could u review again @tmeasday ! 😊

tmeasday

Looks great @Keraito!

Hypnosphi · 2018-06-08T06:19:32Z

Please update test snapshots: yarn test --core --update

Keraito · 2018-06-08T09:23:06Z

I updated the test snapshots, but my 5 added stories for some reason fail with the following error: Invariant Violation: React.Children.only expected to receive a single React element child. Not sure what is exactly causing this

Keraito · 2018-06-08T09:38:21Z

I'm kinda confused at what's going on. I actually updated the storyshots before (29aaa62).

My second commit just now (after running yarn) removed all the updates to the snapshots. The first run of yarn test --core --update however also removed the snapshots update of my added stories and the error described in #3697 (comment) is still present

Keraito · 2018-06-08T15:36:33Z

Nevermind all of that, apperently I just didn't have the dev server running 🤦‍♂️ Should be all good now

Keraito added 3 commits June 1, 2018 15:14

Implement params for info + merge the options and params

a1b4296

Add examples of the param implementation

8b174bc

Update the documentation with the new possibilities

848cf43

Keraito requested review from alterx, dangreenisrael, danielduan, Hypnosphi, igor-dv, theinterned, tmeasday, usulpro and z4o4z as code owners June 1, 2018 14:00

Keraito added feature request addon: info labels Jun 1, 2018

tmeasday reviewed Jun 2, 2018

View reviewed changes

Keraito added 5 commits June 2, 2018 21:33

Change object assign to spread

6217be8

Update storyshots

29aaa62

Change assign to destructure (undefined) and add disable example

0ccf131

Merge branch 'master' into info-params

1a81105

Update doc to clarify the details of how to use decorator

f1d318b

Keraito added 2 commits June 5, 2018 15:36

Update the examples and docs

44ffa26

Deprecate setDefaults

837cfbe

Keraito commented Jun 5, 2018

View reviewed changes

Document disable flag

f1ae4ea

tmeasday approved these changes Jun 6, 2018

View reviewed changes

Hypnosphi and others added 2 commits June 8, 2018 08:20

Merge branch 'master' into info-params

bb3c5bf

Update snapshots

5738a8e

Forgot to run yarn

cd6cdd3

shilman mentioned this pull request Jun 8, 2018

Story Parameters for Official Addons #3625

Closed

6 tasks

Update snapshots correctly

3cc1566

Hypnosphi merged commit 4a250fb into storybookjs:master Jun 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parameters for info addon #3697

Use parameters for info addon #3697

Keraito commented Jun 1, 2018

tmeasday left a comment

tmeasday Jun 2, 2018

tmeasday Jun 2, 2018

shilman Jun 2, 2018

Keraito Jun 2, 2018

tmeasday Jun 4, 2018 •

edited

Loading

tmeasday Jun 2, 2018

tmeasday Jun 2, 2018

Keraito Jun 2, 2018 •

edited

Loading

tmeasday Jun 4, 2018

tmeasday Jun 2, 2018

tmeasday commented Jun 2, 2018

codecov bot commented Jun 2, 2018 •

edited

Loading

Keraito commented Jun 2, 2018

Keraito commented Jun 4, 2018

Keraito Jun 5, 2018

tmeasday Jun 6, 2018

Keraito commented Jun 5, 2018

tmeasday left a comment

Hypnosphi commented Jun 8, 2018

Keraito commented Jun 8, 2018

Keraito commented Jun 8, 2018 •

edited

Loading

Keraito commented Jun 8, 2018

Use parameters for info addon #3697

Use parameters for info addon #3697

Conversation

Keraito commented Jun 1, 2018

Core tests

tmeasday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keraito Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday commented Jun 2, 2018

codecov bot commented Jun 2, 2018 • edited Loading

Codecov Report

Keraito commented Jun 2, 2018

Keraito commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keraito commented Jun 5, 2018

tmeasday left a comment

Choose a reason for hiding this comment

Hypnosphi commented Jun 8, 2018

Keraito commented Jun 8, 2018

Keraito commented Jun 8, 2018 • edited Loading

Keraito commented Jun 8, 2018

tmeasday Jun 4, 2018 •

edited

Loading

Keraito Jun 2, 2018 •

edited

Loading

codecov bot commented Jun 2, 2018 •

edited

Loading

Keraito commented Jun 8, 2018 •

edited

Loading