-
Notifications
You must be signed in to change notification settings - Fork 412
feat: add toBePressed matcher (#203) #658
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
base: main
Are you sure you want to change the base?
feat: add toBePressed matcher (#203) #658
Conversation
Hi, may I ask for reviews? |
Hi @gnapse, can I kindly ask for a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this contribution. It is good stuff. Though there are a couple of things that need to be improved.
(the comments marked as "nit-pick" are not fundamental to be changed, but other ones are)
|
||
### `toBePressed` | ||
|
||
This allows to check whether given element has been [pressed](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows to check whether given element has been [pressed](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed). | |
This allows to check whether given element is [pressed](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed). |
test('throws when element do not have aria-pressed attribute', () => { | ||
const {queryByTestId} = render(`<span data-testid="span" />`) | ||
|
||
expect(() => | ||
expect(queryByTestId('span')).toBePressed(), | ||
).toThrowErrorMatchingInlineSnapshot( | ||
`Only button or input with type="button" or element with role="button" and a valid aria-pressed attribute can be used with .toBePressed()`, | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test perhaps should be titled "throws an error when element is not a button" because that's the real reason for this use case to fail.
To illustrate, it should fail even if you pass the aria-pressed attribute. So maybe also modify the test to show that:
test('throws when element do not have aria-pressed attribute', () => { | |
const {queryByTestId} = render(`<span data-testid="span" />`) | |
expect(() => | |
expect(queryByTestId('span')).toBePressed(), | |
).toThrowErrorMatchingInlineSnapshot( | |
`Only button or input with type="button" or element with role="button" and a valid aria-pressed attribute can be used with .toBePressed()`, | |
) | |
}) | |
test('throws an error when element is not a button', () => { | |
const {queryByTestId} = render(`<span data-testid="span" aria-pressed="true" />`) | |
expect(() => | |
expect(queryByTestId('span')).toBePressed(), | |
).toThrowErrorMatchingInlineSnapshot( | |
`Only button or input with type="button" or element with role="button" and a valid aria-pressed attribute can be used with .toBePressed()`, | |
) | |
}) |
|
||
if (ariaPressed === 'true' || ariaPressed === 'false') { | ||
return true | ||
} | ||
|
||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-pick:
if (ariaPressed === 'true' || ariaPressed === 'false') { | |
return true | |
} | |
return false | |
return ariaPressed === 'true' || ariaPressed === 'false' |
const isButton = () => { | ||
return ( | ||
element.tagName.toLowerCase() === 'button' || | ||
element.getAttribute('role') === 'button' || | ||
(element.tagName.toLowerCase() === 'input' && element.type === 'button') | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly it but misses an edge case. An element can have more than one role via the role attribute (ref).
You could soften the condition on the role attribute to check that it includes the button role:
-element.getAttribute('role') === 'button' ||
+element.getAttribute('role')?.split(/\s/)?.includes('button') ||
Though ideally we should do this via the helper functions found in the to-have-role.js
source file. Unfortunately, those are internal to the module. They'd have to be moved to a new module role-helpers.js
and used from there in that matcher, and in this new matcher.
} | ||
|
||
return { | ||
pass: isButton() && isPressed(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-pick:
I am not sure why you chose to make these (isButton
, isPressed
, isValidAriaElement
) to be functions. They could simply be variables instead. You compute the value once and store it.
Not a big deal, but just curious.
What:
New
toBePressed
matcher is addedWhy:
Closes #203
Checklist:
In my opinion this PR could be followed by another one which would add support for checking
aria-pressed="mixed"
. MaybetoBePartiallyPressed
? It would follow the same approach as splittertoBeChecked
andtoBePartiallyChecked
into two separate matchers.