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.

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

Only make TArgs parameterize args and argTypes in our default annotations #33

Merged
merged 3 commits into from
Dec 15, 2021

Conversation

tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Dec 6, 2021

See this discussion.

In this PR we basically only parameterise args and argTypes by TArgs. All other annotations (the story functions and contexts, the decorators/loaders/play fn) are generic to Args.

The point being is two things:

  • that at any given level (project / component / story) the definition of TArgs you pass in doesn't describe the entire story's args as the three levels are composed together [1]. All it does is describe the args you are defining at that level.

  • you also might want to use decorators/loaders/play functions that are more generic than a single story/component, without having to type cast them[2].

[1] This might be controversial to folks that are trying to be very precise and not use arg inheritance at all.
[2] I'm a little bit bemused you do need to type cast them, we probably need to unpack this a bit more.

@tmeasday tmeasday requested a review from shilman December 6, 2021 06:17
@yannbf
Copy link

yannbf commented Dec 7, 2021

  • These examples should infer args correctly:
const MyStory: LegacyAnnotatedStoryFn<AnyFramework, {disabled: boolean}> = (args) => {}
MyStory.args

This does work for AnnotatedStoryFn

@tmeasday
Copy link
Contributor Author

tmeasday commented Dec 8, 2021

@yannbf so if you use LegacyStoryAnnotation, the first could either be args or the context (thus the any). A better test is either:

// adding a second parameter forces the `ArgsStoryFn`
const MyStory: LegacyAnnotatedStoryFn<AnyFramework, {disabled: boolean}> = (args, context) => {}

// or, simpler, always `ArgsStoryFn`
const MyStory: AnnotatedStoryFn<AnyFramework, {disabled: boolean}> = (args) => {}

I pushed a commit that satisfies what you wanted:

image

@tmeasday
Copy link
Contributor Author

tmeasday commented Dec 8, 2021

@shilman I also removed the play function from the generic BaseAnnotations and only applied it at the story level, which is consistent with prepareStory: https://github.com/storybookjs/storybook/blob/35d0096a772c29bdafac80065232f7728d483b1e/lib/store/src/prepareStory.ts#L196

@shilman
Copy link
Contributor

shilman commented Dec 8, 2021

@tmeasday Why wouldn't we cascade play functions just like any other kind of annotation?

I can imagine wanting to execute a single play function across various args settings, tho i don't have a concrete example at hand. Of course, one could explicitly reuse a single play function on each story, but the same can be said for args/parameters..

@tmeasday
Copy link
Contributor Author

tmeasday commented Dec 8, 2021

@tmeasday Why wouldn't we cascade play functions just like any other kind of annotation?

I'm not sure, but we don't. We can discuss that. No sense in typing it if we don't do it though.

@yannbf
Copy link

yannbf commented Dec 8, 2021

@tmeasday Why wouldn't we cascade play functions just like any other kind of annotation?

I can imagine wanting to execute a single play function across various args settings, tho i don't have a concrete example at hand. Of course, one could explicitly reuse a single play function on each story, but the same can be said for args/parameters..

I actually thought it was possible to run the play function o Meta 🤔
Example use case: Wizard form where the first step is always the same but the second step depends on the component prop. In this case the interaction would be done either like this:

export default {
  play: async () => {
    // the stuff..
  }
}
export FirstScenario = { args: { type: 'first-time-customer' } }
export SecondScenario = { args: { type: 'special-deal-customer' } }
export ThirdScenario = { args: { type: 'vip-customer' } }

or

export default {
}
export FirstScenario = { 
  play: async () => {
    // the stuff..
  },
  args: { type: 'first-time-customer' } 
}
export SecondScenario = {
  play: FirstScenario.play, // in case they don't want to reuse all properties like it would be done with ...FirstScenario
  args: { type: 'special-deal-customer' } 
}
export ThirdScenario = { 
  play: FirstScenario.play,
  args: { type: 'vip-customer' } 
}

@@ -265,7 +260,7 @@ export type ComponentAnnotations<
*
* By defining them each component will have its tab in the args table.
*/
subcomponents?: Record<string, TFramework['component']>;
subcomponents?: Record<string, TFramework['component']>;
Copy link

Choose a reason for hiding this comment

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

unnecessary space became a diff?

@tmeasday
Copy link
Contributor Author

tmeasday commented Dec 9, 2021

I'm OK with making it possible to have a play at the component level. @ghengeveld do you have opinions?

What about at the project level? I think not...

@shilman I think we should merge and release this in SB proper anyways, let's do the component-level play in a follow up set of PRs.

@shilman
Copy link
Contributor

shilman commented Apr 3, 2023

🚀 PR was released in v0.1.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants