feat(ByRole): Allow filter by value state #1223
Conversation
|
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 c9a2416:
|
eps1lon
force-pushed
the
feat/byrole-value
branch
from
c1666a0
to
29f9277
Compare
Codecov Report
@@ Coverage Diff @@
## main #1223 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 999 1035 +36
Branches 323 346 +23
=========================================
+ Hits 999 1035 +36
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 |
eps1lon
force-pushed
the
feat/byrole-value
branch
from
daf92b3
to
29f9277
Compare
eps1lon
changed the title
| if (valueNow !== undefined) { | ||
| // guard against unknown roles | ||
| if ( | ||
| allRoles.get(role as ARIARoleDefinitionKey)?.props['aria-valuenow'] === |
There was a problem hiding this comment.
What do you think about declaring a variable for allRoles.get(role as ARIARoleDefinitionKey) ?
It seems like this is repeated a few times in this meethod.
There was a problem hiding this comment.
I'm fine with repeating stuff. Makes it easier to grok the flow for a single option. If we change this pattern often we can reconsider but for now it seems like it's just an excercise in clean code.
Part of a larger committment to unify APIs for
/react-nativeand/react(which is just/react-domat the moment)What:
Enables
ByRole(role, { value })Note that we don't support
valuenowbut rathervalue.now(same pattern for otheraria-value*attributes). One reason is parity with@testing-library/react-native.The other reason is that
value.*works a bit different in that it looks at allvalue.*and combines them with&&instead of returning on the first match. Today,{ pressed: true, selected: true}has kind of an odd behavior since it only looks at whatever comes first in the implementation. We could combine them with&&as well but it's not clear to me yet that this is always good. So by putting them in a single object makes it clear that we look at the whole object which wouldn't be that obvious for{ name, valuemin, valuemax}. I don't feel too strongly about this specific rationale. I care more about parity with /react-native.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: