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

Select date on IOS UIDatePicker with actionForSetDate #1148

Merged
merged 4 commits into from
Feb 9, 2019

Conversation

matthewrfindley
Copy link
Contributor

@matthewrfindley matthewrfindley commented Feb 8, 2019

  • This is a small change

Description:

Addresses the following issues:
#778
#1051

The problem:

Using setColumnToValue to select the correct date on a DatePickerIOS does not work as expected. This issue defines a solution to select a valid value from the 0 column by using the value of the label. The solution breaks down because the date picker contains the same label for every year in the list. Attempting to select Feb 8 for 2019 would result in selecting Feb 8 for 2006, which is the minimum date in the list.

Example usage of setColumnToValue on DatePickerIOS:

await element(by.type('UIPickerView')).setColumnToValue(0,"Feb 6");

react-native DatePickerIOS renders the date in the UIDatePicker as 2 labels. The first being the 3 letter month abbreviation and day, the second being the 3 letter day abbreviation. Using setColumnToValue breaks down when trying to select the correct date since the implementation of Earl Grey's GREYPickerAction selects only the first label see here. Even if Earl Grey were able to compare using all the labels with a list item, a UIDatePicker would be limited to only 7 years worth of dates. This would not be maintainable since we are not in control of the default range for UIDatePicker, nor is it acceptable to force users to limit the list in their application.

Solution:

Expose an existing Earl Grey action called actionForSetDate within Detox.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 8, 2019

Thanks for submitting this PR. Will review shortly.


@interface GREYActions (Detox)

+ (id<GREYAction>)detoxSetDatePickerDateIOSOnly:(NSString *)dateString withFormat:(NSString *)dateFormat;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not call this "IOSOnly". Should be detoxSetDatePickerDate:withFormat:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

📗 1d6913c


@implementation GREYActions (Detox)

+ (id<GREYAction>)detoxSetDatePickerDateIOSOnly:(NSString *)dateString withFormat:(NSString *)dateFormat
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, detoxSetDatePickerDate:withFormat:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

📗 1d6913c

});

it('can select dates on a UIDatePicker', async () => {
await element(by.type('UIDatePicker')).setDatePickerDateIOSOnly('2019-02-06T05:10:00-08:00', "yyyy-MM-dd'T'HH:mm:ssZZZZZ");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be setDatePickerDate and documentation should be updated to state it is for iOS only, as are the current picker view APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

📗 9470d65

Copy link
Contributor

@LeoNatan LeoNatan left a comment

Choose a reason for hiding this comment

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

Also, please add documentation on the new action here: https://github.com/wix/Detox/blob/master/docs/APIRef.ActionsOnElement.md

detox/src/ios/expect.js Show resolved Hide resolved
@matthewrfindley
Copy link
Contributor Author

@LeoNatan https://jenkins-oss.wixpress.com/job/multi-detox-pr/920/ - Failure seems unrelated to any of the recent changes. 🤷‍♂️

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 9, 2019

Looks good. CI is flaky.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 9, 2019

@rotemmiz Let's merge?

@rotemmiz
Copy link
Member

rotemmiz commented Feb 9, 2019

Android e2e hangs, wonder why

StyleSheet,
DatePickerIOS
} from 'react-native';
import { Text, View, StyleSheet, DatePickerIOS } from 'react-native';
Copy link
Member

@rotemmiz rotemmiz Feb 9, 2019

Choose a reason for hiding this comment

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

You can't add DatePickerIOS on a shared component, it needs to run on both iOS and Android.
You might need to split this screen to .ios.js and .android.js.
This is probably the reason why the e2e hangs, the app crashes.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that’s not a new change. How did it work until now?

Copy link
Member

Choose a reason for hiding this comment

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

ho, you're right, I haven't noticed that

@rotemmiz
Copy link
Member

rotemmiz commented Feb 9, 2019

Please see my comment, I think I found the reason for Android E2E's hangs

@LeoNatan LeoNatan merged commit 6a1d700 into wix:master Feb 9, 2019
@matthewrfindley matthewrfindley deleted the select-date-on-ios branch February 11, 2019 16:22
@lock lock bot locked as resolved and limited conversation to collaborators Feb 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants