feat: add toBePartiallyChecked matcher #249
Conversation
Codecov Report
@@ Coverage Diff @@
## master #249 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 22 +1
Lines 289 305 +16
Branches 69 73 +4
=========================================
+ Hits 289 305 +16
Continue to review full report at Codecov.
|
src/to-be-partially-checked.js
Outdated
| const isValidAriaElement = () => { | ||
| return ( | ||
| element.getAttribute('role') === 'checkbox' && | ||
| element.getAttribute('aria-checked') === 'mixed' |
There was a problem hiding this comment.
This check is already done in isPartiallyChecked probably shouldn't be here, right?
There was a problem hiding this comment.
Yup, I don't think this should be part of this function. This function should only check if the element has a valid role for which this check makes sense. Then the actual check with the boolean value you set the pass property to, that's what then determines if the element is partially checked or not.
There was a problem hiding this comment.
Looks good. Though I'm not approving yet because I want to get your input on my first comment below. Should <input type="checkbox" aria-checked="mixed" /> be considered as partially checked, even though it does not have its indeterminate property set to true?
| describe('.toBePartiallyChecked', () => { | ||
| test('handles input checkbox with aria-checked', () => { | ||
| const {queryByTestId} = render(` | ||
| <input type="checkbox" aria-checked="mixed" data-testid="checkbox-mixed" /> |
There was a problem hiding this comment.
Hmmm, I don't think we should support aria-checked in a <input type="checkbox" />. It should be supported only on non-native checkbox elements with role="checkbox".
The reason being that a <input type="checkbox" /> won't convey to the user visually that it is partially checked. The only way the native checkbox can be partially checked is when its indeterminate property is set to true.
What do you think?
There was a problem hiding this comment.
I thought the same when I tested it in the broswer, as you say indeterminate set to true has a visual effect on the checkbox:
The only thing that made me change my mind is that in the accessibility consoleboth of them had role checkbox and checked mixed, but yeah, I think it should be better to check for input+indeterminate and role checkbox + aria mixed, let me change it
There was a problem hiding this comment.
in the accessibility console both of them had role checkbox and checked mixed
Would you care to share more about this detail?
I'm also particularly interested to know about the input checkbox with indeterminate = true. How is it conveyed in the accessibility console.
There was a problem hiding this comment.
Both inputs show the same data in the Accessibility tab in the Chrome Devtools:
Input with indeterminate:
Input with aria-checked="mixed":
You can check the Codesandbox demo I've been using.
There was a problem hiding this comment.
Ok, my bad then. If the <input type="checkbox" aria-checked="mixed" /> shows as checked: mixed even if indeterminate was not set true, then I think it was ok the first time around, and I assumed without checking what you correctly went out to check.
What do you think?
If you agree, can you easily revert the changes after my earlier review? Sorry for having made you work extra for nothing.
There was a problem hiding this comment.
Yes, I was also asking for your opinion here. But I'm conflicted. The <input type="checkbox" aria-checked="mixed" /> looks visually as if unchecked, but semantically (from the aria perspective) it is still partially checked.
I'm leaning towards trusting the accessibility info. If someone uses an input checkbox with aria-checked="mixed" they would presumably take over the rendering to reflect that using icons and stuff.
So yes, in the end I'm leaning to that (after having thought about it out loud here 😄)
There was a problem hiding this comment.
Yeah, I thought the same at first, since it looks unchecked, but then I thought it's just a matter of how chrome styles the checkbox.
Let's support both of them then :)
There was a problem hiding this comment.
Ok, so the last commit needs to be reverted, right? It was good the first time around, right?
There was a problem hiding this comment.
Yes, I'll revert it and change only the check inside isValidAriaElement
There was a problem hiding this comment.
Should be good now :)
src/to-be-partially-checked.js
Outdated
| const isValidAriaElement = () => { | ||
| return ( | ||
| element.getAttribute('role') === 'checkbox' && | ||
| element.getAttribute('aria-checked') === 'mixed' |
There was a problem hiding this comment.
Yup, I don't think this should be part of this function. This function should only check if the element has a valid role for which this check makes sense. Then the actual check with the boolean value you set the pass property to, that's what then determines if the element is partially checked or not.
|
It should be good now, let me know if I missed something |
|
Not sure why Travis is failing, any idea? |
Not sure either. I re-launched the build and it worked. Will review soon. |
|
@all-contributors add @rubenmoya for code, test, docs |
|
I've put up a pull request to add @rubenmoya! 🎉 |
|
🎉 This PR is included in version 5.8.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
This PR adds a
toBePartiallyCheckedmatcherWhy:
You can read more about the why in #243
How:
Following the implementation of
toBeCheckedbut adapting it to check less elements sinceradioandswitchcannot have a mixed state.Checklist: