-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 duration of element.longPress for iOS #412
Conversation
6409603
to
ec50f11
Compare
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.
Great PR! Thanks !
There are a few point I want to discuss before we merge it, please go over the notes.
detox/src/ios/expect.js
Outdated
super(); | ||
this._call = invoke.callDirectly(GreyActions.actionForLongPress()); | ||
if (duration) { |
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.
let's add default value, drop the condition, and use only GreyActions.actionForLongPressWithDuration
@@ -17,13 +17,18 @@ describe('Actions', () => { | |||
await expect(element(by.text('Long Press Working!!!'))).toBeVisible(); | |||
}); | |||
|
|||
it(':ios: should long press with duration on an element', async () => { |
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.
I don't want to create more discrepancy between iOS and Android API, I'm willing to help you add Android support as well if you need help.
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.
Yeah I also really want this feature can be supported for Android, if you could help will be great. Thanks. :)
detox/test/e2e/c-actions.js
Outdated
it('should multi tap on an element', async () => { | ||
await element(by.id('UniqueId819')).multiTap(3); | ||
await expect(element(by.id('UniqueId819'))).toHaveText('Taps: 3'); | ||
}); | ||
|
||
it('should tap on an element at point', async () => { | ||
await element(by.id('View7990')).tapAtPoint({x:180, y:140}); | ||
await element(by.id('View7990')).tapAtPoint({x:180, y:160}); |
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.
Why was that changed ?
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.
I added a button for new test case, so the UniqueId819
button position was changed.
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.
👍
5984004
to
0f18872
Compare
generation/earl-grey/index.js
Outdated
@@ -47,6 +47,7 @@ if ( | |||
|
|||
// Constants | |||
const SUPPORTED_TYPES = [ | |||
"CFTimeInterval", |
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.
🎉🎉🎉
How is this progressing? Can I help somehow? We pretty much need this feature for both platforms. :) |
Alright, I started working on this feature based on this PR. |
@jozan, for Android? |
For iOS, we need this feature eventually for Android as well but even
getting iOS to work would be very nice.
I tried to rebase to master but it did not work out of the box since lot
has changed past three months especially in generation dir. Also, after
rebase both longPress and longPress(duration) did not pass tests and failed
to error from this line:
https://github.com/wix/detox/blob/master/detox/ios/Detox/MethodInvocation.m#L225
where value i was 0.
I don’t have time to debug further today.
…On Sun, 4 Feb 2018 at 22.31, Rotem Mizrachi-Meidan ***@***.***> wrote:
@jozan <https://github.com/jozan>, for Android?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#412 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAXsqY4PksoiZHmNxM3_YnH-F0mjDShOks5tRhONgaJpZM4QglCG>
.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Go away stale bot! Sorry for the huge delay. We have intention to merge this in. |
Any news on this PR? Let’s merge for iOS and then in the future add for Android? |
Yes, let's do it |
@jhen0409 Could you please merge master and see that tests are not broken? |
Thanks! |
@yershalom Jenkins is flaky, please look why. |
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.
Thanks for this PR.
I saw some style changes. Any reason those are needed?
detox/src/ios/expect.js
Outdated
@@ -65,9 +65,9 @@ class TapAtPointAction extends Action { | |||
} | |||
|
|||
class LongPressAction extends Action { | |||
constructor() { | |||
constructor(duration = 750) { |
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.
Why is the default 750? Is this what Earl Grey uses in the long press action?
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.
I forgot the reason, but it should be 1000 for now.
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.
Let's instead use the default. If no param is provided, let's call the current API, but if there is provided, let's call the new API.
694e7ed
to
996eb24
Compare
The some style changes caused by Prettier, I reverted it. |
Cool. Let’s wait for the CI and I’ll merge. Thank you for your contribution! |
996eb24
to
5f1b6b0
Compare
5f1b6b0
to
72233dc
Compare
Related to #410. I also added the test case
Long Press Me 1.5s
.