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

Add testID support to android datepicker #705

Conversation

mauricedoepke
Copy link
Contributor

Summary

Makes testID prop work on Android as well.

This makes testing DatePickers with detox easier. Till now this was irrelevant, as detox did not support setting dates on datepickers on Android.
I am currently working on a PR enabling this very functionality though: wix/Detox#3790
Together with that PR it would be very useful having testID support for DatePickers on Android.

Test Plan

The existing tests are sufficient. I only changed the matchers to not use the android className anymore, but the testID instead. This implicitely tests that the testID is working.

Compatibility

OS Implemented when
iOS already implemented before
Android implemented by this PR

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow) to the best of my abbillity. Please check whether I did it proper way.
  • I added a sample use of the API in the example project (example/App.js) Already covered by existing example app.
  • I have added automated tests, either in JS or e2e tests, as applicable. I slightly adjusted the e2e tests to cover this feature.

@@ -22,6 +22,7 @@ export default class DatePickerAndroid {
* - `value` (`Date` object) - date to show by default
* - `minimumDate` (`Date` object) - minimum date that can be selected
* - `maximumDate` (`Date` object) - maximum date that can be selected
* - `testID` (`string`) - testID for usage with e.g. detox
Copy link

@noomorph noomorph Dec 30, 2022

Choose a reason for hiding this comment

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

I guess it is called accessibility tags on Android.

I'd suggest to be less specific and replace "detox" with "automation frameworks".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noomorph I just rephrased it. What do you think about it now?

README.md Outdated
#### `View Props` (`optional`, `iOS only`)

On iOS, you can pass any [View props](https://reactnative.dev/docs/view#props) to the component. Given that the underlying component is a native view, not all of them are guaranteed to be supported, but `testID` and `onLayout` are known to work.
On iOS, you can pass any [View props](https://reactnative.dev/docs/view#props) to the component. Given that the underlying component is a native view, not all of them are guaranteed to be supported, but `onLayout` is known to work.

Choose a reason for hiding this comment

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

Well, it doesn't look like testID stops working after your PR, so why did you have to rephrase here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from this section to give it its own, right above. This section talks about ios specific props and with this PR testID will work on both android and ios. I thought that was easier to convey by moving testID into its own section.

Feel free to suggest an improvement though.

@noomorph
Copy link

@mauricedoepke did you run it, did that work?

@mauricedoepke
Copy link
Contributor Author

mauricedoepke commented Jan 12, 2023

@mauricedoepke did you run it, did that work?

@noomorph Yes I ran the android tests locally and they worked for me.

I was not able to run the ios tests though. Neither on my branch nor on master. The demo app stayed white for me on the ios simulator.

I guess the circle ci tests have to be aproved to run in this repo? Because the error seems to be auth related.

@vonovak
Copy link
Member

vonovak commented Jun 22, 2023

@mauricedoepke would you mind rebasing this on top of master?

The repo now uses the latest version of detox, so the tests should now pass. Thank you! :)

@vonovak vonovak force-pushed the add-testID-support-to-android-datepicker branch from 999e41e to 423b0c5 Compare July 6, 2023 11:40
@vonovak vonovak merged commit e571d71 into react-native-datetimepicker:master Jul 6, 2023
@vonovak
Copy link
Member

vonovak commented Jul 6, 2023

@mauricedoepke thank you for your PR! For some reason GH is showing my icon on the commits as I have rebased them, but you're still the author 🙂

@mauricedoepke
Copy link
Contributor Author

@vonovak Thats no problem for me. Thank you for taking the time to merge it.

vonovak pushed a commit that referenced this pull request Jul 6, 2023
# [7.4.0](v7.3.0...v7.4.0) (2023-07-06)

### Features

* **android:** add testID support to date (not time) picker ([#705](#705)) ([e571d71](e571d71))
@vonovak
Copy link
Member

vonovak commented Jul 6, 2023

🎉 This issue has been resolved in version 7.4.0 🎉

If this package helps you, consider sponsoring us! 🚀

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

Successfully merging this pull request may close these issues.

3 participants