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

Refactor transitional decorator from addon-notes #3559

Merged
merged 8 commits into from
May 15, 2018
+191 −33
Merged

Refactor transitional decorator from addon-notes #3559

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
98a99b5
Refactor transitional decorator from addon-notes
tmeasday May 8, 2018
e7f7f32
Ensure that `addDecorator(decorator(options))` is *NOT* marked deprec…
tmeasday May 14, 2018
15c2ed8
Fix addon-notes test
tmeasday May 14, 2018
b8f8acb
Update CLI snapshots
tmeasday May 15, 2018
199098c
Rename to `makeDecorator`
tmeasday May 15, 2018
de1c65c
Merge remote-tracking branch 'origin/master' into tmeasday/refactor-t…
tmeasday May 15, 2018
5d86249
Added test for `makeDecorator`
tmeasday May 15, 2018
a2e57cf
Added `skipIfNoParametersOrOptions` argument to `makeDecorator`
tmeasday May 15, 2018
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
34 changes: 11 additions & 23 deletions addons/notes/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import addons from '@storybook/addons';
import addons, { makeTransitionalDecorator } from '@storybook/addons';
import marked from 'marked';

function renderMarkdown(text, options) {
marked.setOptions({ ...marked.defaults, options });
return marked(text);
}

const decorator = options => {
const channel = addons.getChannel();
return (getStory, context) => {
const {
parameters: { notes },
} = context;
const storyOptions = notes || options;
export const withNotes = makeTransitionalDecorator({
name: 'withNotes',
parameterName: 'notes',
wrapper: (getStory, context, { options, parameters }) => {
const channel = addons.getChannel();

const storyOptions = parameters || options;

if (storyOptions) {
const { text, markdown, markdownOptions } =
Expand All @@ -26,23 +26,11 @@ const decorator = options => {
}

return getStory(context);
};
};

const hoc = options => story => context => decorator(options)(story, context);
},
});

export const withMarkdownNotes = (text, options) =>
hoc({
withNotes({
markdown: text,
markdownOptions: options,
});

export const withNotes = (...args) => {
// Used without options as .addDecorator(withNotes)
if (typeof args[0] === 'function') {
return decorator()(...args);
}

// Input are options, ala .add('name', withNotes('note')(() => <Story/>))
return hoc(args[0]);
};
3 changes: 2 additions & 1 deletion lib/addons/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
},
"dependencies": {
"@storybook/channels": "4.0.0-alpha.4",
"global": "^4.3.2"
"global": "^4.3.2",
"util-deprecate": "^1.0.2"
}
}
1 change: 1 addition & 0 deletions lib/addons/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import global from 'global';

export mockChannel from './storybook-channel-mock';
export { makeTransitionalDecorator } from './transitional-decorator';

export class AddonStore {
constructor() {
Expand Down
34 changes: 34 additions & 0 deletions lib/addons/src/transitional-decorator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import deprecate from 'util-deprecate';

// Create a decorator that can be used both in the (deprecated) old "hoc" style:
// .add('story', decorator(options)(() => <Story />));
//
// And in the new, "parameterized" style:
// .addDecorator(decorator)
// .add('story', () => <Story />, { name: { parameters } });

export const makeTransitionalDecorator = ({ name, parameterName, wrapper }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename it to something like makePolymorphicDecorator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! Or just makeDecorator()?

Copy link
Member

Choose a reason for hiding this comment

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

just makeDecorator sounds good

Copy link
Member

Choose a reason for hiding this comment

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

I like the makeDecorator name too

const decorator = options => (getStory, context) => {
const parameters = context.parameters && context.parameters[parameterName];

return wrapper(getStory, context, {
options,
parameters,
});
};

const hoc = deprecate(
options => story => context => decorator(options)(story, context),
`Passing stories directly into ${name}() is deprecated, instead use addDecorator(${name}) and pass options with the '${parameterName}' parameter`
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that we want to deprecate the old way? Honestly, it looks a lot more explicit to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I thought we did?

I think the strongest argument in favour is if you want to pass parameters to more than one decorator for the same story:

.add('story', withNotes('foo')(backgrounds(['red', 'white'])(() => <Story />)))

vs

.add('story', () => <Story />, { notes: 'foo', backgrounds: ['red', 'white'] })

I was planning on changing all the documentation to not mention the old way at least, so deprecating made sense to me, but I'm open to whatever.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could afford supporting both

@ndelangen WDYT

Copy link
Member

Choose a reason for hiding this comment

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

What is a benefit of having the old API supported?

Copy link
Member

Choose a reason for hiding this comment

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

You may want to supply the same parameters (e.g. backgrounds) to multiple stories

And even extract it into variable or module, like here:
https://github.com/storybooks/storybook/tree/master/addons/backgrounds#usage
("Of course it's easy to create a library module")

Copy link
Member

Choose a reason for hiding this comment

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

I mean my idea is that you would just add addDecorators(backgrounds) once in your .storybook/config.js once (like you need to add it to .storybook/addons.js), and then you would add the parameters to turn the addon on wherever you needed it.

That's perfect I think.

Copy link
Member

Choose a reason for hiding this comment

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

@tmeasday I just realised that those are two different usecases

// here withSomething is params => (storyFn, context) => output
.addDecorator(withSomething(params))

// here it is params => storyFn => () => output
.add('story', withSomething(params)(storyFn))

Actually addons that support both do some checks to figure out which usage is it.

Can we maybe just deprecate the latter but keep the former, because it's quite useful in cases like backgrounds?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, now I see that the latter is the thing we're actually deprecating here, good

Copy link
Member

Choose a reason for hiding this comment

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

But the former would probably need to be supported when migrating other addons

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I see what you are saying here. Let me take another pass at this.

);

return (...args) => {
// Used without options as .addDecorator(decorator)
if (typeof args[0] === 'function') {
return decorator()(...args);
}

// Input are options, ala .add('name', decorator('note')(() => <Story/>))
return hoc(args[0]);
};
};