Skip to content

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

Merged
thymikee merged 12 commits into
callstack:masterfrom
ryanjwessel:master
Jul 21, 2020
Merged

thymikee merged 12 commits into
callstack:masterfrom
ryanjwessel:master

Conversation

@ryanjwessel

@ryanjwessel ryanjwessel commented Jul 17, 2020 •

Copy link
Copy Markdown
Contributor

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.

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@mdjastrzebski mdjastrzebski left a comment •

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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.

@mdjastrzebski mdjastrzebski left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/__tests__/a11yAPI.test.js Resolved
Comment thread src/__tests__/a11yAPI.test.js Resolved
Comment thread src/__tests__/a11yAPI.test.js Resolved
Comment thread src/__tests__/a11yAPI.test.js Resolved
@ryanjwessel

Copy link
Copy Markdown
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.

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member

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 👍

Comment thread CONTRIBUTING.md Resolved
Comment thread src/__tests__/a11yAPI.test.js Resolved
@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
Copy Markdown
Member

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

@ryanjwessel

Copy link
Copy Markdown
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