-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
♻️ Migrate stories 54..61 to args
#37860
♻️ Migrate stories 54..61 to args
#37860
Conversation
args: { | ||
width: '640px', | ||
height: '360px', | ||
ariaLabel: 'Video Player', |
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.
nit: use aria-label
so you don't have to extract it and rename it in the fixtures
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.
using a hyphen in an arg name gives me a linting error, i dont think i can get around extracting and renaming in this case.
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.
you can turn off the rule with an eslint-disable-next-line comment otherwise feel free to merge
}; | ||
|
||
Default.argTypes = { | ||
amount: { |
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.
nit/fmi: why do you use args for some and argTypes for others? amount: 1
is equivalent to {name: 'amount', control: {type: 'number'}, defaultValue: 1,}
, right?
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.
Yep thats correct. The difference is that storybook will infer a control type based on the initial value of the arg and for argTypes we specify the control type. In this case since these would all probably be inferred correctly they really could all be args.
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.
some (non-blocking) comments
inferred i.e. strings, numbers, and booleans.
Warning: disparity between this PR Percy build and its The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build /
|
Partial for #35923.