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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

barrymichaeldoyle
Copy link

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.

@github-actions
Copy link

Hey @barrymichaeldoyle! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

@netlify
Copy link

netlify bot commented Aug 25, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit b1f76a3
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/64fed3f22a7041000851f8dc
😎 Deploy Preview https://deploy-preview-11559--react-navigation-example.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (e432c32) 75.77% compared to head (b1f76a3) 75.77%.

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           
Files Changed Coverage Δ
packages/elements/src/Header/Header.tsx 74.07% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@osdnk osdnk left a 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?

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.

None yet

2 participants