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

Support menuitemcheckbox/menuitemradio roles in toBeChecked #266

Closed
tf opened this issue Jun 23, 2020 · 6 comments · Fixed by #267
Closed

Support menuitemcheckbox/menuitemradio roles in toBeChecked #266

tf opened this issue Jun 23, 2020 · 6 comments · Fixed by #267
Labels
enhancement New feature or request released

Comments

@tf
Copy link
Contributor

tf commented Jun 23, 2020

Describe the feature you'd like:

According to the spec menuitemcheckbox and menuitemradio both require aria-checked attributes.

I think it would be nice to be able to write:

expect(getByRole('menuitemradio', {name: 'Some option'})).toBeChecked();

Suggested implementation:

Add menuitemcheckbox and menuitemradio to the allowed roles in the matcher.

Describe alternatives you've considered:

So far, I was only able to use low level matchers like toHaveAttribute('aria-checked', 'true').

Teachability, Documentation, Adoption, Migration Strategy:

The roles would need to be added to list of allowed roles mentioned in the readme and the error messages.

Happy to prepare a PR.

@nickmccurdy nickmccurdy added the enhancement New feature or request label Jun 23, 2020
@tf
Copy link
Contributor Author

tf commented Jun 24, 2020

Looking at the spec of aria-checked again, I realized it would probably make sense to also add option andtreeitem, the two other roles which support the attribute, while we are at it.

@eps1lon
Copy link
Member

eps1lon commented Jun 24, 2020

It's probably best to leverage aria-query if you want to make sure aria attributes are supported on a given role.

@tf
Copy link
Contributor Author

tf commented Jun 24, 2020

Great idea. If it's fine to add the dependency, then the check could probably be implemented similar to the way it is used in dom-testing-library. At the moment the error message explicitly lists all permitted roles. Do we want to preserve this behavior? If yes, should we iterate over all to find the ones supporting the attribute or is there a more convenient way?

@gnapse
Copy link
Member

gnapse commented Jun 24, 2020

+1 to this feature request, and +1 to using something like aria-query for not having to maintain this logic ourselves.

At the moment the error message explicitly lists all permitted roles. Do we want to preserve this behavior? If yes, should we iterate over all to find the ones supporting the attribute or is there a more convenient way?

I'd say that, unless it becomes cumbersome while implementing this to achieve it, it'd be nice if we can preserve that bit of functionality. At worse we can discuss and decide in the context of the PR when someone jumps on implementing this.

@tf
Copy link
Contributor Author

tf commented Jun 25, 2020

I've submitted PR #267. Happy to incorporate feedback.

@gnapse
Copy link
Member

gnapse commented Jun 25, 2020

🎉 This issue has been resolved in version 5.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants