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
feat: added headerTitleTestID prop to HeaderOptions and implemented it #11559
base: main
Are you sure you want to change the base?
Conversation
Hey @barrymichaeldoyle! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines. |
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #11559 +/- ##
=======================================
Coverage 75.77% 75.77%
=======================================
Files 200 200
Lines 5817 5817
Branches 2271 2271
=======================================
Hits 4408 4408
Misses 1362 1362
Partials 47 47
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While your proposal is solid, I'm thinking it might be best to put it on hold for now. I'm keen on avoiding a single-location implementation and potentially missing out on other suitable spots for it.
If you're up for it, we could explore a middle ground—perhaps something like adding testId to the entire header, including Drawer, TabBar, etc. This way, it becomes a versatile solution, and folks can write tests based on the text using the withAncestor
annotation.
What do you think?
Motivation
Solves this issue: #9232 (and many others like it)
When writing e2e tests with detox, there is no easy way to target the
headerTitle
other than getting the element by text. but in a lot of cases it is better to get the element by testID,Test plan
This is quite a basic change, linting and existing tests pass. There aren't any existing tests setup for the Header component, if there were I would add a test case.