-
Notifications
You must be signed in to change notification settings - Fork 91
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-group): add a11y, visual tests #1346
Conversation
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* fix(btn-group): add fix for buttons in forms * add tests (needs baseline images rendered * update baseline images
PLEASE DON'T FAIL!
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.
Somehow the button group radio in the docs is now broken for me (cannot even focus on it). I haven't investigated further though.
For the tests I can see that the setup for the a11y and the visual tests is pretty similar. I would recommend to create something like button-group.test.setup.ts
so that there is no need to duplicate many line of codes. You could also tests the form
variations in the a11y tests if you want which probably makes sense (???).
If you go with the .setup.ts
file approach we may want to add the *.setup.ts
in this tsconfig to avoid creating unnecessary type for the production build.
I understand that we are introducing a small breaking change here both in Stacks Classic and Stacks Editor (which will depend on consumers upgrading the stacks classic css they use in their page).
Do you feel comfortable merging this change as part of v 2.1.0 or shall we wait for a future v2.2.0? I am asking because I would like to don't rush this through if possible due to enterprise release fixes we need to ship.
Apart from those minor things, the rest looks good.
Thanks @dancormier for adding the tests 🎉
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.
Few changes to make before we are able to merge this one.
Will circle back here when you finish the cleanup. :)
Thanks
I removed any reference to the `s-btn-group__radio` variant and all but one `form` test
@giamir I've updated this PR with your suggested changes. Since your last review I:
Note The default, form, and radio test images render identically. This is as intended to test those unique children within the button group. |
This seems unnecessary since the visual element is `.s-btn` regardless of whether it's a radio group or a standard 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.
Outstanding work. Reads much better than before and great catch on those a11y failures. 🎉
I took the liberty to remove an unnecessary eslint-disable
rule from the test setup.
Good to be merged :)
This PR adds visual and accessibility tests for the
s-btn-group
component.@giamir since we'll need to update Core to support the new button group design, would this be a good opportunity to also include the minor class syntax breaking change noted above?In the button group redesign implementation, the
radio
modifieron
s-btn-group` no longer provides any unique styling. Consumers should remove any references to this modifier, but including it will not affect anything materially.