Skip to content

feat: Add toBeChecked matcher #141

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

Merged
merged 2 commits into from
Oct 25, 2019

Conversation

connormeredith
Copy link
Contributor

@connormeredith connormeredith commented Oct 8, 2019

Closes #107

What:
Added a toBeChecked matcher.

Why:
It was not implemented in #90 but I thought it would be a nice addition as there currently isn't a matcher to check the value of a checkbox without it being part of a form.

How:
Implemented along the same lines as toHaveValue except it's only limited to <input type="checkbox />.

I wasn't too sure on the best way to handle <input type="radio" /> as it can be "checked" and also have a "value", so I've left that for now suggesting to use toHaveFormValue instead - similar to how toHaveValue handles this situation.

Checklist:

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

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.

Thanks for jumping on contributing this.

It is actually a feature that was requested a while ago, and initially dismissed, but recently it turned out we may need it. I do not see you referenced the original issue where it was requested (#107) so that' why I'm giving all this intro.

Your implementation is good, but from that original issue request you can see that this may be helpful also for radio buttons, even if they also have a value. And most importantly, to make this really useful, it would be great if it also supported custom checkboxes, e.g. something like this:

<div role="checkbox" aria-checked="true|false" />

Do you think you can expand this work to support that (and if we support <input type="radio" /> then we should support custom aria radio buttons as well).

@connormeredith
Copy link
Contributor Author

Thanks for the extra info @gnapse, I missed the initial issue. I'm happy to add <input type="radio" /> and support for custom aria radio buttons and checkboxes. I'll push some changes as soon as I can 👍

@connormeredith
Copy link
Contributor Author

I've added support for type="radio" and role="checkbox|radio". My knowledge of aria-checked is vague so I'm happy to make further changes to make it compliant with the spec if it isn't already. I've also not included the "indeterminate" state as per @kentcdodds comment on the original issue, I can always add it at a later date if it's needed 🙌

@connormeredith
Copy link
Contributor Author

@gnapse I've updated with master, is there anything else you need me to do to get this merged?

@gnapse gnapse merged commit ed0b272 into testing-library:master Oct 25, 2019
@gnapse
Copy link
Member

gnapse commented Oct 25, 2019

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@testing-library testing-library deleted a comment from allcontributors bot Oct 25, 2019
@gnapse
Copy link
Member

gnapse commented Oct 25, 2019

@all-contributors please add @connormeredith for code, test, doc

@allcontributors
Copy link
Contributor

@gnapse

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

@michaelmior
Copy link

Given that the goal for extra matchers is readability, I would expect the function to be called toBeSelected for radio buttons, as I don't ever see radio buttons referred to as being "checked" but "selected."

@gnapse
Copy link
Member

gnapse commented Oct 28, 2019

Given that the goal for extra matchers is readability, I would expect the function to be called toBeSelected for radio buttons, as I don't ever see radio buttons referred to as being "checked" but "selected."

@michaelmior while I get your point, I'm not sure I agree. Both the api at the native radio button level and the aria- side of things seem to agree that it is "checked" and not "selected". On the aria- side of things aria-checked is supported in elements with role="radio", while aria-selected is used for different purposes and it's not even documented to make sense in a role="radio" element.

What do you think based on all this?

@michaelmior
Copy link

That's a fair point. I guess I retract my statement in light of that :)

@Belco90
Copy link
Member

Belco90 commented Jan 16, 2020

Just realized about this matcher was included. It's amazing, thank you!

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.

Add toBeChecked for <input type="radio" /> and <input type="checkbox" />
4 participants