Query by role and accessibilityState and improve ByRole query errors - #1161
Conversation
mdjastrzebski
left a comment
mdjastrzebski
left a comment
There was a problem hiding this comment.
Kudos for implementing this feature @AugustinLF
-
This implementation should automatically support upcoming
aria-statesprops, when checked on host components, as they are taken into consideration when passing them from exported composite component. See here, not sure if these are present in the current released version of RN. -
It seems that some components take into account other props as well, when calcualting
accessibilityStatethat gets passed to host component, e.g.TouchableOpacityalso checksdisabledprop. The current implementation already handles that but we might add some tests to verify it.
There was a problem hiding this comment.
Can we simplify this test to only contain a single TouchableOpacity with disabled={true}?
There was a problem hiding this comment.
Those three RN component use different implementation behind the hood, so my goal was to test all of them (TouchableOpacity and TouchableHighlight extend TouchableWithoutFeedback), and I felt like covering our bases.
On the other hand I do see your point, since we're just testing RN internal implementation.
I'll keep the three ones, if you feel strongly about that (or splitting it in three tests) let me know).
There was a problem hiding this comment.
It makes sense to test all three TouchablesXxx and I would also metion Pressable in that context. Maybe you can use test.each to automate testing over these component variants?
There was a problem hiding this comment.
I tried, but the difference in types (Button doesn't use children), it leads to pretty ugly code, and I'd say that it's not worth it, it'll make the tests harder to read, and here it's fairly simple.
There was a problem hiding this comment.
The overall structure looks correct now 💯
I've added some comments related to various implementation details to polish things out.
We also need to have our documentation updated to account for new options.
I've logged a separate #1165 to fix make *ByA11yStates to have the same matching behaviour as in this PR.
|
@mdjastrzebski should have addressed all your comments I believe! |
|
@AugustinLF I'll need pause a couple of days on open source work as I've got cold-like symptoms. I'll review the PRs as soon as I get better. |
mdjastrzebski
left a comment
mdjastrzebski
left a comment
There was a problem hiding this comment.
I've done a final round of review. Overall looks very good for me:
- All the logic seems correct
- we have a comprehensive set of tests
I've left two readability/style final comments & requested to document behaviour around defaulting or not to false for each prop.
I didn't give approve, only because as I worry that @thymikee could merge it before addressing these ;-)
|
@mdjastrzebski a bit delayed, but everything should have been addressed now :) |
2f11472 to
3e01734
Compare
mdjastrzebski
left a comment
mdjastrzebski
left a comment
There was a problem hiding this comment.
Awesome work @AugustinLF!
*ByRole gets really powerful these days: first name, and now a11y states
|
Let's merge it! |
|
Thanks for the help! When are we shipping a new release, we've had at least two improvements to |
|
I could do a release on Friday if I will have enough time |
|
@AugustinLF we've published 11.3.0 a couple of days ago, but I just created a release docs in GH: https://github.com/callstack/react-native-testing-library/releases/tag/v11.3.0 |
2 participants
Summary
RTL does have some form of validation to make sure that we don't use incompatible combinations (roles/states that don't make sense, like a checked button), but I'm not sure about that, and it is out of scope, we can create an additional issue if we feel like it.
Closes #1134