-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
React: Add DecoratorFn type to exports #8121
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-react-decorator.storybook.now.sh |
@hipstersmoothie @kroeder Do we need to add this to every other framework as well? |
I am unfamiliar with the other frameworks but it would probably make sense. Would you like me to add it to the other apps? |
@hipstersmoothie if you could add it to the other frameworks too, that'd be great, but I think we can do that in a separate PR tbh, no reason to block this one for that. |
@@ -27,6 +27,7 @@ export const storiesOf: ClientApi['storiesOf'] = (kind, m) => { | |||
|
|||
export const configure: ClientApi['configure'] = (...args) => api.configure(...args, framework); | |||
export const addDecorator: ClientApi['addDecorator'] = api.clientApi.addDecorator; | |||
export type StoryDecorator = Parameters<typeof addDecorator>[0]; |
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.
TIL
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 personally think the name StoryDecorator is a bit verbose, either DecoratorFn or DecoratorFunction would be a bit better IMHO.
This IS what it's called in the addons package btw.
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 am open to change the name. I was just copying what the definitely typed package named it to cause the least friction. But will change it to anything you want as it's pretty easy to replace it in my stories.
Automention: Hey @emilio-martinez @gaetanmaisse @kroeder, you've been tagged! Can you give a hand here? |
@hipstersmoothie what do you think of the proposed namechange? |
I am open to change the name. I was just copying what the DefinitelyTyped package named it to cause the least friction. But will change it to anything you want as it's pretty easy to replace it in my stories. |
Yeah I'd rather change the name to something that makes sense to us. |
@ndelangen changed it to |
React: Add DecoratorFn type to exports
Renaming |
@mvasin is there something we can do to make it back-compat? want to create a PR for that? |
@mvasin Technically it's not a breaking change, since the types were not in this repo yet and were maintained externally. |
What I did
With the old
@types/storybook__react
, users could useStoryDecorator
to make a decorator outside of theaddDecorator
function. With the new types I had to do a little type math to get it to workThis PR add
Decorator
to the@storybook/react
types.How to test
If your answer is yes to any of these, please make sure to include it in your PR.