Skip to content

feat: introduce ByLabelText, ByRole and ByHintText a11y aliases #445

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

Merged
merged 12 commits into from
Jul 21, 2020

Conversation

ryanjwessel
Copy link
Contributor

@ryanjwessel ryanjwessel commented Jul 17, 2020

Summary

Users migrating from @testing-library/react-native will have to update their accessibility queries due to a slightly different naming convention. This PR adds aliases for the @testing-library/react-native queries to be usable in this library.

This is motivated by the thread on Issue #442

Test plan

I am not sure about testing this. I can make additions to this PR if needed. The test suite passes, and I see a11yAPI.test.js uses only the a11y version of the queries.

To Note

The queries for placeholder text are not one-to-one between the two libraries, but I don't think those are set up for aliasing yet. All of the @testing-library/react-native placeholder queries are appended with Text, whereas this library does not do that.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Great start! Now we need a simple test (copy what we have already) and update typings alongside with their tests (for TypeScript, check typings directory)

@ryanjwessel
Copy link
Contributor Author

Thanks @thymikee! I duplicated the tests and updated the types. I'm not quite sure what typings/__tests__/index.test.tsx is doing, but I updated that as well. I can revert that change if needed.

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

nit: No sure if I would actually duplicate the unit tests for function aliases though. People reading code might be confused whether these are aliases or actual functions. Please put the expects from new query aliases next to expects from original A11y queries in the respective test suits.

Duplicated typescript typing "tests" are fine. As this is different.

@ryanjwessel
Copy link
Contributor Author

Fair point @mdjastrzebski. I think they should fall under the same test blocks since they are covering the same functionality. I'll update this now.

@ryanjwessel
Copy link
Contributor Author

I have consolidated the tests, but there were some issues with this. I added some FIXME tags where the tests were failing due to the consolidation. Two out of three seem to have trouble with async during ...AllBy queries. I haven't had time to dig into why though.

Also, I updated the docs to include these new queries.

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Spotted few aliases to rename.

@ryanjwessel
Copy link
Contributor Author

Thanks for your review @thymikee and @mdjastrzebski! I just pushed an updated a11yAPI test file that asserts referential equality for the query aliases. The test suite is passing, and the diff is a lot simpler now. 😄

Please let me know if there are any other changes either of you feel are necessary.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good, left one more note to consider 👍

@thymikee thymikee changed the title Provide a11y query aliases to assist users migrating from @testing-library/react-native feat: introduce ByLabelText, ByRole and ByHintText a11y aliases Jul 21, 2020
@thymikee thymikee merged commit 61359c7 into callstack:master Jul 21, 2020
@thymikee
Copy link
Member

Thank you very much for you contribution @ryanjwessel, appreciated!

@ryanjwessel
Copy link
Contributor Author

Thank you both for your help! This was my first ever contribution to OSS, so I am really grateful for your guidance in contributing to this project. Glad I was able to contribute!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants