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

React: Add ComponentStory convenience type #14780

Merged
merged 6 commits into from May 10, 2021
Merged

Conversation

shilman
Copy link
Member

@shilman shilman commented May 1, 2021

Issue: N/A

What I did

Added a convenience type, ComponentStory for the common case of a story that spreads args directly into a React component.

- // Before
- const Template: Story<React.ComponentProps<typeof TsButton>> = (args) => <TsButton {...args} />;
+ // After
+ const Template: ComponentStory<typeof TsButton> = (args) => <TsButton {...args} />;

@tooppaaa @gaetanmaisse @ndelangen @tmeasday WDYT?

@yannbf also has some other ideas which are documented here

How to test

CI passes

@nx-cloud
Copy link

nx-cloud bot commented May 1, 2021

Nx Cloud Report

CI ran the following commands for commit 99ddea0. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@yannbf
Copy link
Member

yannbf commented May 1, 2021

This is a great addition, and it will work as well for components which have inline props like:

const Button = ({ label, color }: { label: string, color: string }) => <.../>

Adding to that, in the typescript playground that michael shared, you will find both solutions:

1 - Story<typeof Component> (under Example 2)
Extension of the existing Story type, making it flexible enough to handle either Story<Args> or a Story<typeof Component> scenarios.

2 - ComponentStory<typeof Component> (under Example 3)
Type specifically created to extract props of a component. Won't and shouldn't work as ComponentStory<Args>.

A couple of things to consider:

  • Which solution is less confusing and might bring best value to the users
  • Should we already handle the Strict type here? (see discussion from fix: dont mutate Story prop types to optional #14550), and depending if we do, we might have either Story and StrictStory types, or Story, StrictStory, ComponentStory and StrictComponentStory.

@yannbf
Copy link
Member

yannbf commented May 3, 2021

@ndelangen what are your thoughts? :D

@ndelangen
Copy link
Member

I like it, the only thing I'm unsure about is the addition of a new versioned ts-definition file. I'm not sure that's required considering the definition is an addition anyway?

@shilman
Copy link
Member Author

shilman commented May 3, 2021

@ndelangen happy to remove it -- just did it for consistency (figuring we'd collapse it all at some point anyway)

@ndelangen
Copy link
Member

Not a blocker @shilman

@yannbf
Copy link
Member

yannbf commented May 6, 2021

@shilman Just thought of another scenario: Meta, which also receives args as generic. Will we end up Story, ComponentStory, Meta, ComponentMeta, StrictStory, StrictComponentStory, StrictMeta and StrictComponentMeta?

@shilman
Copy link
Member Author

shilman commented May 7, 2021

@yannbf Great point. I'll add ComponentMeta to the PR. I don't think we need StrictComponent{Meta,Story} -- users can construct that from Strict{Story,Meta} by hand. If that becomes the popular way to type stories, we can revisit the type system in 7.0. WDYT?

@yannbf
Copy link
Member

yannbf commented May 9, 2021

@yannbf Great point. I'll add ComponentMeta to the PR. I don't think we need StrictComponent{Meta,Story} -- users can construct that from Strict{Story,Meta} by hand. If that becomes the popular way to type stories, we can revisit the type system in 7.0. WDYT?

Sounds good! Let's do it

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

Successfully merging this pull request may close these issues.

None yet

4 participants