Skip to content
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

feat: add toBePartiallyChecked matcher #249

Merged
merged 2 commits into from
May 19, 2020
Merged

feat: add toBePartiallyChecked matcher #249

merged 2 commits into from
May 19, 2020

Conversation

rubenmoya
Copy link
Contributor

What:

This PR adds a toBePartiallyChecked matcher

Why:

You can read more about the why in #243

How:

Following the implementation of toBeChecked but adapting it to check less elements since radio and switch cannot have a mixed state.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

@codecov
Copy link

codecov bot commented May 10, 2020

Codecov Report

Merging #249 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/to-be-partially-checked.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c9e8e5...487ba22. Read the comment docs.

const isValidAriaElement = () => {
return (
element.getAttribute('role') === 'checkbox' &&
element.getAttribute('aria-checked') === 'mixed'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is already done in isPartiallyChecked probably shouldn't be here, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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

Copy link
Member

@gnapse gnapse May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both inputs show the same data in the Accessibility tab in the Chrome Devtools:

Input with indeterminate:

image

Input with aria-checked="mixed":

image

You can check the Codesandbox demo I've been using.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the last commit needs to be reverted, right? It was good the first time around, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll revert it and change only the check inside isValidAriaElement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good now :)

const isValidAriaElement = () => {
return (
element.getAttribute('role') === 'checkbox' &&
element.getAttribute('aria-checked') === 'mixed'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rubenmoya
Copy link
Contributor Author

It should be good now, let me know if I missed something

@rubenmoya rubenmoya requested a review from gnapse May 12, 2020 07:30
@rubenmoya
Copy link
Contributor Author

Not sure why Travis is failing, any idea?

@gnapse
Copy link
Member

gnapse commented May 19, 2020

Not sure why Travis is failing, any idea?

Not sure either. I re-launched the build and it worked. Will review soon.

@gnapse gnapse merged commit e46299b into testing-library:master May 19, 2020
@gnapse
Copy link
Member

gnapse commented May 19, 2020

@all-contributors add @rubenmoya for code, test, docs

@allcontributors
Copy link
Contributor

@gnapse

I've put up a pull request to add @rubenmoya! 🎉

@gnapse
Copy link
Member

gnapse commented May 19, 2020

🎉 This PR is included in version 5.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

@andycarrell andycarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants