Use per-story parameters in Notes addon #3373

tmeasday · 2018-04-06T03:37:13Z

Now we have per-story parameters, we can change the addon APIs to be more ergonomic via the use of parameters.

This PR is a first pass at changing the notes addon to use a parameter named notes. I want to put it out there to try and flush out any problems with the approach and get feedback before following up with changing the other addons.

New API:

Instead of:

storiesOf(..)
  .add(
    'using decorator arguments, withNotes',
    withNotes('Notes into withNotes')(() => <Story/>)
  );

We now do

storiesOf(..)
  .add('withNotes', () => <Story/>, {
    notes:
      'This is the notes for a button. This is helpful for adding details about a story in a separate panel.',
  })

To test

See the official-storybook.

Use official-storybook stories

Hypnosphi · 2018-04-09T00:14:13Z

Wow, so cool! Can we do the same for info?

Hypnosphi · 2018-04-09T00:15:46Z

addons/notes/README.md


 ```js
-import { storiesOf } from '@storybook/react';
+import { configure, addDecorator } from '@storybook/react`; // <- whichever storybook version you use


The "version" word is a bit confusing here =)

Hypnosphi · 2018-04-09T00:18:11Z

addons/notes/src/index.js

-    channel.emit('storybook/notes/add_notes', marked(text));
+  return (getStory, context) => {
+    const { parameters: { notes } } = context;
+    console.log({ options, getStory, context, notes });


Left by mistake?

tmeasday · 2018-04-09T00:28:59Z

Can we do the same for info?

Yes, definitely, as well as some others.

I just wanted to get this out there and find out what people thought. For instance, are we OK with addons just claiming parameter namespaces (notes in this case)? Obviously there is scope for collision, but I am guessing people can be sensible about it.

Any other objections?

If not I will put a few utilities together that make this easier to do in each case and do the obvious things in the other addons.

tmeasday · 2018-04-09T01:40:27Z

Would like to see some explicit 👍 from some other interested folks before moving forward here.

danielduan

Interesting. It's not that different from what we used to be doing so I wouldn't call this an API revamp by any means. It is slightly better so I definitely welcome this tweak.

I think for an API v2, we should think about an API that resembles more like the content being rendered. I'm not a huge fan of these multiple function parameters in the .add() variants. The chaining of functions is also a bit bizarre withNotes()(). The way we declare storiesOf() is also a bit strange, and its also easy to miss the module parameter at the end as well.

Looks fine to me.

igor-dv

Very cool.

If this PR is targeting the v4, probably, it worth removing unneeded exports.

igor-dv · 2018-04-09T05:06:26Z

examples/official-storybook/stories/addon-notes.stories.js

-  )
+    `;
+
+storiesOf('Addons|Notes', module)


Please add these example to the other frameworks too

tmeasday · 2018-04-09T07:07:42Z

it worth removing unneeded exports.

@igor-dv do you mean the old WithNotes component (slated for removal in 4.0), or the support for withNotes('note')(story) / withMarkdownNotes('note')(story)? As much as 4.0 is a breaking release, I think we should have a period of deprecation..

Either we deprecate in an upcoming 3.4.x release (in which case this all needs to be backported to 3.4, probably not a great plan), or we leave the current API around (marked deprecated) until 5.0. I'm OK with that, really.

tmeasday · 2018-04-09T07:09:24Z

OK, I'll call that a quorum. Will finish off this PR then follow it up with similar changes to other addons.

Hypnosphi · 2018-04-09T08:45:29Z

we should think about an API that resembles more like the content being rendered

You should probably take a look at https://github.com/storybooks/SBNext/blob/POC-bundler/examples/react/in/button/button.example.js

tmeasday · 2018-04-10T01:10:49Z

@Hypnosphi / @danielduan I expect the concept of parameters to come through to the new API however---people will still need a way to annotate their "pure" examples with instructions for SB/documentation.

This isn't the place to discuss the exact API but one idea might be to add a property to the exported function:

export const example1 = () => <Component>with children</Component>;

example1.storyParameters = {
  notes: 'This is the story note that will appear in the storybook UI'
}

codecov · 2018-04-13T10:09:57Z

Codecov Report

Merging #3373 into master will increase coverage by 0.03%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #3373      +/-   ##
==========================================
+ Coverage   36.65%   36.69%   +0.03%     
==========================================
  Files         458      458              
  Lines       10124    10135      +11     
  Branches      895      886       -9     
==========================================
+ Hits         3711     3719       +8     
- Misses       5854     5877      +23     
+ Partials      559      539      -20

Impacted Files	Coverage Δ
app/mithril/src/client/preview/render.js	`0% <ø> (ø)`	⬆️
addons/notes/src/index.js	`90% <88.88%> (+32.85%)`	⬆️
addons/notes/src/react.js	`0% <0%> (-33.34%)`	⬇️
lib/ui/src/modules/ui/libs/filters.js	`48.64% <0%> (ø)`	⬆️
addons/jest/src/hoc/provideJestResult.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/markdown/pre/pre.js	`77.58% <0%> (ø)`	⬆️
lib/codemod/src/transforms/update-addon-info.js	`50% <0%> (ø)`	⬆️
addons/storysource/src/loader/index.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/types/Enum.js	`0% <0%> (ø)`	⬆️
addons/storysource/src/loader/parsers/parser-ts.js	`14.28% <0%> (ø)`	⬆️
... and 61 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 e8281ab...bd0eb7f. Read the comment docs.

tmeasday · 2018-04-13T10:10:01Z

Updated the remaining app layers and addressed the other things on this ticket.

tmeasday · 2018-04-13T10:11:30Z

Also removed WithNotes (old currently-deprecated API).

Next up I will factor out a utility to make this easy to generalize and deprecate the current API as part of that.

tmeasday added 2 commits April 6, 2018 12:27

Updated addon-notes to add params based app.

fdafcbf

Use official-storybook stories

Update addon-notes README

b490fa4

tmeasday requested review from alexandrebodin, danielduan, Hypnosphi and usulpro as code owners April 6, 2018 03:37

tmeasday requested a review from a team April 6, 2018 03:37

tmeasday added addon: notes feature request labels Apr 6, 2018

tmeasday changed the title ~~Update addon-notes README~~ Use per-story parameters in Notes addon Apr 8, 2018

Hypnosphi reviewed Apr 9, 2018

View reviewed changes

Hypnosphi approved these changes Apr 9, 2018

View reviewed changes

Hypnosphi reviewed Apr 9, 2018

View reviewed changes

danielduan approved these changes Apr 9, 2018

View reviewed changes

igor-dv approved these changes Apr 9, 2018

View reviewed changes

tmeasday added 4 commits April 13, 2018 19:51

Small fixes from PR

88212b2

Remove deprecated WithNotes for 4.0

194d6cd

Use new API in all addon-notes stories

c2fa6c3

Render stories with context in mithril

3cb0823

tmeasday requested review from alterx and naipath as code owners April 13, 2018 10:09

tmeasday added 2 commits April 13, 2018 20:23

Update tests with new API

83cc95b

Update storyshots and fix cra too

e2f2c4b

tmeasday requested a review from ndelangen as a code owner April 13, 2018 10:31

Hypnosphi approved these changes Apr 13, 2018

View reviewed changes

Merge branch 'master' into tmeasday/use-parameters-in-addons

bd0eb7f

Hypnosphi merged commit 80c4bf5 into master Apr 13, 2018

Hypnosphi deleted the tmeasday/use-parameters-in-addons branch April 13, 2018 23:45

shilman mentioned this pull request May 22, 2018

Story Parameters for Official Addons #3625

Closed

6 tasks

dependencies bot mentioned this pull request Jan 15, 2021

Update 25 dependencies from npm #2724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use per-story parameters in Notes addon #3373

Use per-story parameters in Notes addon #3373

tmeasday commented Apr 6, 2018 •

edited

Loading

Hypnosphi commented Apr 9, 2018

Hypnosphi Apr 9, 2018

Hypnosphi Apr 9, 2018

tmeasday Apr 9, 2018

tmeasday commented Apr 9, 2018

tmeasday commented Apr 9, 2018

danielduan left a comment •

edited

Loading

igor-dv left a comment

igor-dv Apr 9, 2018

tmeasday Apr 9, 2018

tmeasday commented Apr 9, 2018

tmeasday commented Apr 9, 2018

Hypnosphi commented Apr 9, 2018

tmeasday commented Apr 10, 2018

codecov bot commented Apr 13, 2018 •

edited

Loading

tmeasday commented Apr 13, 2018

tmeasday commented Apr 13, 2018

Use per-story parameters in Notes addon #3373

Use per-story parameters in Notes addon #3373

Conversation

tmeasday commented Apr 6, 2018 • edited Loading

New API:

To test

Hypnosphi commented Apr 9, 2018

Hypnosphi Apr 9, 2018

Choose a reason for hiding this comment

Hypnosphi Apr 9, 2018

Choose a reason for hiding this comment

tmeasday Apr 9, 2018

Choose a reason for hiding this comment

tmeasday commented Apr 9, 2018

tmeasday commented Apr 9, 2018

danielduan left a comment • edited Loading

Choose a reason for hiding this comment

igor-dv left a comment

Choose a reason for hiding this comment

igor-dv Apr 9, 2018

Choose a reason for hiding this comment

tmeasday Apr 9, 2018

Choose a reason for hiding this comment

tmeasday commented Apr 9, 2018

tmeasday commented Apr 9, 2018

Hypnosphi commented Apr 9, 2018

tmeasday commented Apr 10, 2018

codecov bot commented Apr 13, 2018 • edited Loading

Codecov Report

tmeasday commented Apr 13, 2018

tmeasday commented Apr 13, 2018

tmeasday commented Apr 6, 2018 •

edited

Loading

danielduan left a comment •

edited

Loading

codecov bot commented Apr 13, 2018 •

edited

Loading