feat(ByRole): Allow filter by disabled state #1221
Conversation
eps1lon
force-pushed
the
feat/byrole-disabled
branch
from
2209724 to
eeebfc2
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a42f42e:
|
Codecov Report
@@ Coverage Diff @@
## main #1221 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 1038 1049 +11
Branches 346 353 +7
=========================================
+ Hits 1038 1049 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| <button /> | ||
| </div>`, | ||
| ) | ||
| expect(getByRole('button', {disabled: true})).toBeInTheDocument() |
There was a problem hiding this comment.
Just an idea, maybe for the completeness of this test, we should also add:
expect(queryByRole('button', {disabled: false})).not.toBeInTheDocument()
| @@ -281,6 +281,29 @@ function computeAriaCurrent(element) { | |||
| ) | |||
| } | |||
|
|
|||
| const elementsSupportingDisabledAttribute = new Set([ | |||
There was a problem hiding this comment.
I might be missing something, but this seems to me like something that should be defined in aria-query because it can change as part of the spec and having it here is like mixing the ARIA logic with our role implementation.
Wdyt?
There was a problem hiding this comment.
Also, I see we have something like this in jest-dom too, so it makes me think that this indeed shouldn't belong in one of our packages.
There was a problem hiding this comment.
I don't want to put more stuff into aria-query. dom-accesibility-api makes more sense at this point. Feel free to raise a PR.
There was a problem hiding this comment.
Done: eps1lon/dom-accessibility-api#919
Please lmk if I can change stuff, I'm not happy with putting the change in the util file but I'm not familiar with the codebase, so I'm happy to change it.
Thanks!
| ? // https://www.w3.org/TR/html-aam-1.0/#html-attribute-state-and-property-mappings | ||
| true | ||
| : // https://www.w3.org/TR/wai-aria-1.1/#aria-disabled | ||
| element.getAttribute('aria-disabled') === 'true' |
There was a problem hiding this comment.
I think it's worth adding this to jest-dom assertion too (I can do this on my own if you agree with me).
Having two libraries in the organization considering different things as disabled is quite confusing IMO.
There was a problem hiding this comment.
Querying should not be used for assertions. For jest-dom i.e. there was a clear case why this wasn't as straight forward because it might contain footguns.
/dom: queries based on a11y tree
For assertions I'd rather remove the convenience query from jest-dom altogether in favor of an explicit query for just a11y disabled.
There was a problem hiding this comment.
I agree with that querying and asserting are two different operations.
Just so I'll understand, what are the footguns you're referring to?
I don't think removing the toBeDisabled assertion is a good idea as it's widely used but maybe I'm missing something.
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
|
Would be super helpful to have this |
Part of a larger committment to unify APIs for
/react-nativeand/react(which is just/react-domat the moment)What:
Enables
ByRole(role, { disabled })Why:
Makes writing tests for react-native and react-dom easier by removing barriers when changing the target platform that's being tested.
How:
Same patterns as for expanded, selected etc
Checklist: