feat: Add CheckBoxGroup/Dialog/RadioGroup test utils #9039
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
changed the title
|
Build successful! 🎉 |
| } | ||
|
|
||
| if (!this.checkboxgroup.contains(document.activeElement)) { | ||
| act(() => checkboxes[0].focus()); |
There was a problem hiding this comment.
Should we do this by default? It can sometimes cause behaviour that doesn't match real life. Maybe there should be an option to force focus, but otherwise we throw if they aren't in the checkbox group already?
There was a problem hiding this comment.
From feedback for other users and my own experience from converting tests in Quarry to use the test utils, there is often a lot of friction when people are integrating the utils into the flow of their existing test and expect that calling these "toggle/trigger" functions should just work, even if active focus is in a separate place in their app and the real life use case would be to press Tab 4x to get to the checkboxgroup/etc. I think ideally everyones tests would simulate the exact user flows but realistically I'd like there to be as little friction as possible in adoption.
There was a problem hiding this comment.
makes sense, i was on the fence about it anyways. if they have a test not working at some point down the line, i think we can suggest they alter the flow to a real user one and see if it still happens
| if (dialog && document.activeElement !== this._trigger && dialog.contains(document.activeElement)) { | ||
| this._dialog = dialog; | ||
| } else { | ||
| // TODO: is it too brittle to throw here? |
There was a problem hiding this comment.
seems fine? were you running into unexpected cases?
There was a problem hiding this comment.
mainly still unsure if timing might throw off this check, but seems ok so far
There was a problem hiding this comment.
cool, lets remove the TODO
| act(() => this._radiogroup.focus()); | ||
| } | ||
|
|
||
| let radioOrientation = this._radiogroup.getAttribute('aria-orientation') || 'horizontal'; |
There was a problem hiding this comment.
seems like keyboardNavigateToRadio should determine the orientation, no need to pass it in?
There was a problem hiding this comment.
ya I might refactor this when I refactor some common flows out of the utils into generalized patterns (aka keyboardNavigateToElement might become a common util instead)
There was a problem hiding this comment.
ok, just be mindful of breaking changes 👍🏻
|
|
||
| let dialog = getByRole('dialog'); | ||
| let dialogTrigger = document.activeElement! as HTMLElement; | ||
| // TODO: this is a weird case where the popover isn't actually linked to the button, behaving more like a modal |
There was a problem hiding this comment.
good point, should this be attached?
There was a problem hiding this comment.
yeah, should it be a popover? Then again https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/examples/datepicker-dialog/ is similar so maybe not a problem
There was a problem hiding this comment.
maybe we just wait on this and see if anyone brings up a strong feeling. for now we can remove the comment i think
|
Build successful! 🎉 |
3 participants
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
If you wanna test the S2 RadioGroup fix, simply go to the S2 storybook and set it to horizontal + rtl. ArrowLeft/Right should move focus to the left and right as expected.
🧢 Your Project:
RSP