-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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
Refactoring of withX wrappers withNotes and withInfo #1538
Conversation
- make signatures consistent - make string-only variant of withNotes/withInfo - simplify withInfo code
addons/notes/src/index.js
Outdated
const channel = addons.getChannel(); | ||
|
||
const text = typeof textOrOptions === 'string' ? textOrOptions : textOrOptions.text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract into a function
@@ -135,8 +135,8 @@ storiesOf('Button', module) | |||
) | |||
.add( | |||
'addons composition', | |||
withInfo('see Notes panel for composition info')( | |||
withNotes({ notes: 'Composition: Info(Notes())' })(context => | |||
withInfo({ text: 'see Notes panel for composition info' })( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the example we show to users, do you think it makes sense like this?
I think the less verbose syntax:
withInfo('see Notes panel for composition info')
makes more sense tbh.
We should document these options somewhere, but the example should showcase the common use case here, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
addWithInfo: deprecate(function addWithInfo(storyName, info, storyFn, _options) { | ||
return this.add(storyName, withInfo(info, _options)(storyFn)); | ||
addWithInfo: deprecate(function addWithInfo(storyName, text, storyFn, options) { | ||
if (typeof storyFn !== 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a bit of refactoring,
in some cases the const text
is actually the storyFn
. I find this confusing.
I'd prefer using abstract names or simply an argument array to do detection, and only assign to a const when we are sure what it is.
const options = { | ||
...defaultOptions, | ||
..._options, | ||
...infoOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also merge in:
if (!options.propTables) {
options.propTables = null;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you merge this in? see the comment above in the code.
This will be backwards compatible? or not? |
I think both |
addons/info/src/index.js
Outdated
export const withInfo = (info, _options) => | ||
addonCompose((storyFn, context) => addInfo(storyFn, context, info, _options)); | ||
export const withInfo = textOrOptions => { | ||
if (typeof textOrOptions === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just:
const options = (typeof textOrOptions === 'string') ?
{ text: textOrOptions } :
textOrOptions;
Codecov Report
@@ Coverage Diff @@
## master #1538 +/- ##
==========================================
+ Coverage 20.52% 20.55% +0.02%
==========================================
Files 241 241
Lines 5222 5220 -2
Branches 643 657 +14
==========================================
+ Hits 1072 1073 +1
+ Misses 3667 3639 -28
- Partials 483 508 +25
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
||
return getStory => context => { | ||
// send the notes to the channel before the story is rendered | ||
channel.emit('storybook/notes/add_notes', notes); | ||
channel.emit('storybook/notes/add_notes', options.text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we mean that this addon could have other options in the future (or even if we want to keep it as an addon example) why don't pass all 'options' object here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can emit those options once we handle them on the other end.
There may be "preview-side" options, so I don't think it's necessarily the case that we'd want to emit them all.
@@ -34,20 +28,10 @@ const defaultMarksyConf = { | |||
ul: UL, | |||
}; | |||
|
|||
export function addInfo(storyFn, context, info, _options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to not export this function to users? Maybe our documentation shouldn't encourage people to use it, but there could be some "advanced" use cases when this form is more convenient! Especially it makes sense after #1501
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we should never export a function to users if we don't need to, because then we are stuck supporting it unless we want to break the API.
@@ -83,12 +67,25 @@ export function addInfo(storyFn, context, info, _options) { | |||
); | |||
} | |||
|
|||
export const withInfo = (info, _options) => | |||
addonCompose((storyFn, context) => addInfo(storyFn, context, info, _options)); | |||
export const withInfo = textOrOptions => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it would be nice to provide here such an option as well:
withInfo('text info', { options })
There are two reasons for that:
1.1. We might want to use different text info
with the same other options:
.add('Button', withInfo('Default Button', widgetInfoOptions)(() => <Button />))
1.2. We might want to use multiline template string for `text info`
and such syntax would be less verbose
- It's relevant to the previous behavior of
.addWithInfo
when we could pass (or not)text info
as a separate argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usulpro the reason @tmeasday wanted to keep it to a single argument is for this API:
https://gist.github.com/shilman/792dc25550daa9c2bf37238f4ef7a398#options-types
The API described in the gist is obviously just a proposal, but we think having a single argument makes stuff like this simple to implement. If we have two arguments it gets complicated fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue: #1452
What I did
withX
signatures consistentI have documented the rationale behind this change here:
https://gist.github.com/shilman/792dc25550daa9c2bf37238f4ef7a398#options-types
Will obviously put this into a more permanent place in the docs if we converge on this convention and migrate things over to it.
How to test