Menu: migrate Storybook examples to CSF3 #68204
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Hiding whitespace changes removes a lot of noise while reviewing
ciampo
added
[Type] Code Quality
|
Flaky tests detected in 980c20e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12455042305
|
| args: { | ||
| children: ( |
There was a problem hiding this comment.
The csf-2-to-3 codemod would use render: () => { ... } instead of args.children. What's the difference? Seems like it allows you to specify the props type inline when passing it to render:
export const Default: StoryObj< typeof Menu > = {
render: ( props: MenuProps ) => (
<Menu { ...props }>
/// ...
</Menu>
),
args: {},
};
One difference to note is that the codemod uses render: but also specifically adds this to all stories that inherit from Default:
args: {
...Default.args,
},
There was a problem hiding this comment.
When I was working on some of the TypeScript compilation hotspots, I noticed that not having a render function is better for performance, since it can bypass a lot of work. So we should avoid render and prefer children when possible.
There was a problem hiding this comment.
As Lena points out, the reason why I used children at the beginning was because I thought it'd be more idiomatic / more performant.
| string[] | ||
| >( [ 'b' ] ); | ||
| export const WithCheckboxes: Story = { | ||
| render: function WithCheckboxes( props: MenuProps ) { |
There was a problem hiding this comment.
Why do we need to provide a named function here? Can't we just do:
render: ( props: MenuProps ) => (
<>
//...
</>
),
There was a problem hiding this comment.
I used named functions to prevent warnings about using hooks outside of a component.
| args: { | ||
| children: ( |
There was a problem hiding this comment.
When I was working on some of the TypeScript compilation hotspots, I noticed that not having a render function is better for performance, since it can bypass a lot of work. So we should avoid render and prefer children when possible.
| string[] | ||
| >( [ 'b' ] ); | ||
| export const WithCheckboxes: Story = { | ||
| render: function WithCheckboxes( props: MenuProps ) { |
There was a problem hiding this comment.
These parameter types shouldn't be necessary, since that is already handled through the StoryObj<> generic.
| render: function WithCheckboxes( props: MenuProps ) { | |
| render: function WithCheckboxes( props ) { |
There was a problem hiding this comment.
I did that initially, but then I added back the explicit props because it seems to improve TypeScript performance (TS doesn't need to compute the prop types since they are explicitly declared)
| </Menu> | ||
| ); | ||
| Default.args = {}; | ||
| type Story = StoryObj< MenuProps >; |
There was a problem hiding this comment.
Any reason it's not this?
| type Story = StoryObj< MenuProps >; | |
| type Story = StoryObj< typeof Menu >; |
There was a problem hiding this comment.
Same as #68204 (comment) — performance reasons.
Although I'm happy to revert if we don't think performance is improved with this change.
ciampo
force-pushed
the
feat/menu-storybook-csf3
branch
from
2514423 to
fd4b118
Compare
What?
Migrate
Menu's storybook examples to the latest story formatWhy?
Keeping aligned with latest Storybook specs and features, potentially better performance
How?
Followed docs at https://storybook.js.org/docs/api/csf
Testing Instructions
Build storybook locally (
npm run storybook:dev), make sure that allMenustories work as ontrunk