-
Notifications
You must be signed in to change notification settings - Fork 215
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
test(button): Add static state tables for all Button components #469
Conversation
); | ||
|
||
storiesOf('Components|Buttons/Button/React/Visual Testing/Button', module) | ||
.addParameters({component: Button}) |
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.
Should we put all of these under a States
folder since we might also add a story for theming for each one of these?
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.
There is a States
story for each component. I went for the following organization.
Visual Testing/
Button/
States
Theming
IconButton/
States
Theming
...
If the consensus is to swap the component and states/theming/etc categories, I can do that.
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
7e92ca5
to
7d0340b
Compare
These tables make our button component look pretty buggy. Wondering if we should change the target to the |
af2948a
to
5a55ae8
Compare
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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.
I think we can merge as soon as they are added to chromatic right?
Summary
In preparation for #468, we would like to have static state tables in place so that we can ensure no regressions are introduced in the upcoming button refactor. This PR adds them for
Button
,DropdownButton
, andTextButton
. It also moves the existingIconButton
static states story to a common folder with the others (I thought this was clearer than having to dive into subfolders for each visual testing story - especially when we start to add multiple stories for theming).Note: There are actually quite a few issues with some of the prop combinations. Some problems are stylistic, while others make it clear where we've exposed possible prop combinations that shouldn't be available. I would propose we leave them as is for now and try to address them in the refactor. Would love some input on this.