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.

Sign up for GitHub

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

Jump to bottom

fix: *ByA11yState default value false value for busy, disabled & selected state #1166

Merged
merged 8 commits into from
Oct 21, 2022
Merged

fix: *ByA11yState default value false value for busy, disabled & selected state #1166

merged 8 commits into from
Oct 21, 2022

Conversation

mdjastrzebski
Copy link
Member

Summary

Resolves #1165.

Based on empirical research results: https://github.com/callstack/react-native-testing-library/wiki/Accessibility:-State

Test plan

Added additional test for matching behaviour.

@mdjastrzebski mdjastrzebski mentioned this pull request Oct 9, 2022
Closed
7 tasks
@AugustinLF
Copy link
Collaborator

AugustinLF commented Oct 10, 2022 •
edited
Loading

Might be worth sharing an implementation with #1161, no?

@mdjastrzebski mdjastrzebski added bug Something isn't working a11y labels Oct 15, 2022
@mdjastrzebski mdjastrzebski marked this pull request as ready for review October 15, 2022 22:03
@mdjastrzebski
Copy link
Member Author

@AugustinLF Added exported matchAccessibilityState that might be shared with *ByRole implementation.

pierrezimmermannbam
pierrezimmermannbam suggested changes Oct 19, 2022
View reviewed changes
src/queries/a11yState.ts Outdated Show resolved Hide resolved
src/queries/a11yState.ts Outdated Show resolved Hide resolved
@mdjastrzebski
Copy link
Member Author

@AugustinLF would you mind if I would use the exported matchAccessibilityState predicate from *ByA11yState in *ByRole to have common logic? Seems like *ByA11yState and *ByRole share the same behaviour in terms of a11y state matching.

@mdjastrzebski mdjastrzebski force-pushed the fix/by-a11y-state-default-value branch from 11fa4a0 to 864528c Compare October 20, 2022 09:53
@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam I've addressed your comment about default state object.

@mdjastrzebski
Copy link
Member Author

@AugustinLF I've used matchAccessibilityState from *ByA11yState to reduce duplicated state matching in *ByRole query.

@AugustinLF
Copy link
Collaborator

would you mind if I would use the exported matchAccessibilityState predicate from *ByA11yState in *ByRole to have common logic? Seems like *ByA11yState and *ByRole share the same behaviour in terms of a11y state matching.

@mdjastrzebski Yep good for me, makes sense

AugustinLF
AugustinLF reviewed Oct 20, 2022
View reviewed changes
src/queries/role.ts Outdated Show resolved Hide resolved
@AugustinLF
Copy link
Collaborator

Nice work!

pierrezimmermannbam
pierrezimmermannbam approved these changes Oct 21, 2022
View reviewed changes
Copy link
Collaborator

@pierrezimmermannbam pierrezimmermannbam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdjastrzebski I added just an additional suggestion of refactor, otherwise changes look good to me, well done !

@mdjastrzebski mdjastrzebski merged commit bd5ddd7 into main Oct 21, 2022
@mdjastrzebski mdjastrzebski deleted the fix/by-a11y-state-default-value branch October 21, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierrezimmermannbam pierrezimmermannbam pierrezimmermannbam approved these changes

@AugustinLF AugustinLF AugustinLF approved these changes

@thymikee thymikee Awaiting requested review from thymikee

Assignees
No one assigned
Labels
a11y bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: *ByA11yState query ignores default state values of false
3 participants
@mdjastrzebski @AugustinLF @pierrezimmermannbam