-
Notifications
You must be signed in to change notification settings - Fork 219
Add storybook demo for CheckboxControl #3030
Conversation
@opr is this ready for review? I see the diff is quite big (+28k, -19k) and I'm not sure what files need to be reviewed, any clues? I think maybe undoing the merge and instead rebasing this branch with trunk would simplify the diff? Otherwise, what about cherry picking the relevant commits into a new branch based on trunk? I'm a bit wary of merging a PR as big as this one, specially considering github-actions reports some assets increase. |
8754658
to
503776d
Compare
Size Change: 0 B Total Size: 1.17 MB ℹ️ View Unchanged
|
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.
Thanks for updating the PR, @opr! The story works great and code looks good. I just added a minor comment about naming the component, but not a blocker.
/** | ||
* Internal dependencies | ||
*/ | ||
import Component from '../'; |
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.
It seems like in other stories we are importing the components with their name (in this case, it would be CheckboxControl
), and the story has a generic name (in some that I checked, it was named Default
). Do you think we should keep it consistent here?
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.
Yes I agree! I'll make this change
This keeps our stories more consistent
Part of #3031
Accessibility
All animations respectprefers-reduced-motion
Screenshots
How to test the changes in this Pull Request:
npm run storybook
@base-components
>CheckboxControl
>CheckboxControl