react-native-datetimepicker / datetimepicker

React Native date & time picker component for iOS, Android and Windows
MIT License
2.53k stars 413 forks source link

fix: Tests without scrolling #903

Closed JoshBarnesD closed 5 months ago

JoshBarnesD commented 5 months ago

Summary

This PR aims to fix the need for tests to contain scrolling to accommodate the recent changes to the example app's App.js file. This stems from a short conversation that took place in #902.

First off I have reverted the test changes done in #902, specifically those that added scrollTo('bottom') or scrollTo('top'). I then implemented some small styling changes to App.js that reduces the separation between the different inputs on the main screen. Lastly, I changed the test devices to an iPhone 15 Pro Max for iOS and a Pixel 6 Pro for Android, both of which are larger and provide enough space to show all elements of the test App without scrolling.

Please note that for Android, we simply state the TestingAVD name in .detoxrc, which Im pretty sure does not change the actual test device used in CircleCI. This means that we would probably need to change the configured emulator for CircleCI, something I don't think I can do or at least can't see where...

Test Plan

Android Tests

Screenshot 2024-06-04 at 10 56 01 Tested with Pixel 6 Pro API 30

iOS Tests

Screenshot 2024-06-04 at 11 43 54 Tested with iPhone 15 Pro Max with iOS 17.5

NOTE: It seems that while locally I am running iOS 17.5, CircleCI is running an iPhone14 with version 16.4. It would seem that the Native TimePicker varies between these two versions and causes the tests to fail with the later version of iOS... Not sure what we should do about this. Maybe choosing a specific version and adapting the tests to it might be the best course of action.

Compatibility

OS Implemented
iOS
Android

Checklist

vonovak commented 5 months ago

quick comment: the android device for circleCI is specified in https://github.com/react-native-datetimepicker/datetimepicker/blob/f30dce94d56f7fa290a0c7ee2c750ca564c857da/.circleci/config.yml#L102

I'll take a proper look later. Thank you!

JoshBarnesD commented 5 months ago

Ah, I'm completely blind, thanks for pointing this out, not sure how I didn't see this... I can change the specified devices and push if that's ok?

vonovak commented 5 months ago

this looks good to me, so please feel free to merge if you're done with it :). However, when merging (after you press the green button), please change the commit message to start with chore because the way it's now, this would bump the version and trigger a release of the library even though there isn't anything new for the users, it's just an internal change :) Thank you! :)

vonovak commented 5 months ago

:tada: This issue has been resolved in version 8.1.1 :tada:

If this package helps you, consider sponsoring us! :rocket: