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 x,y coordinate support for swipe action #2432

Merged
merged 5 commits into from
Nov 19, 2020
Merged

Add x,y coordinate support for swipe action #2432

merged 5 commits into from
Nov 19, 2020

Conversation

noomorph
Copy link
Collaborator

@noomorph noomorph commented Oct 23, 2020

  • This change has been discussed in issue Add offsetPositionX and offsetPositionY to swipe(direction, speed, percentage) #2428 and the solution has been agreed upon with maintainers.
  • JS native implementation
  • iOS native implementation
  • Android native implementation
  • Fix code review comments
  • Ensure consistent parameter naming
  • Add a few more tests for various fast=true/false, startPointX/Y=NaN/something, offset=NaN/0/0.5/1 tests
  • Re-cover Android implementation with unit-tests

Description:

Signature:

/*
 * @param { 'up' | 'down' | 'left' | 'right' } direction - Direction to swipe
 * @param { 'fast' | 'slow' } speed
 * @param {number} [normalizedSwipeOffset] - swipe amount relative to the screen width/height
 * @param {number} [normalizedStartingPointX] - X coordinate of swipe starting point, relative to the view width
 * @param {number} [normalizedStartingPointY] - Y coordinate of swipe starting point, relative to the view height
 */
function swipe(direction, speed, normalizedSwipeOffset = NaN, normalizedStartingPointX = NaN, normalizedStartingPointY = NaN) { ... }

Usage:

await element(by.id('ScrollView161')).swipe('up');
await element(by.id('ScrollView161')).swipe('down', 'slow');
await element(by.id('ScrollView161')).swipe('left', 'fast', 0.95);
await element(by.id('ScrollView161')).swipe('right', 'slow', NaN, 0.3);
await element(by.id('ScrollView161')).swipe('up', 'fast', undefined, 0.5, 0.95);

@LeoNatan LeoNatan changed the title feat(iOS): add x,y coordinate support for swipe action Add x,y coordinate support for swipe action Oct 23, 2020
@LeoNatan
Copy link
Contributor

Implemented on iOS. Is this iOS only for now or will @d4vidi add impl for Android as well?

@d4vidi
Copy link
Collaborator

d4vidi commented Oct 25, 2020

I can get to it soon, should be fairly ez to set up

@LeoNatan
Copy link
Contributor

Great, thanks! @d4vidi

@noomorph
Copy link
Collaborator Author

noomorph commented Nov 7, 2020

@d4vidi, @LeoNatan, need your advice regarding the swiped amount.

Look at the difference between the current iOS and Android implementations.

This happens because I calculate on Android the end coordinate not relative to the screen, but relative to the element itself. I have done that not really on purpose, just I was trying to follow the logic in the current Android implementation, and ended up in the place where I am.

So, iOS and Android behavior here differs. What would be more correct behavior, what do you think?

@LeoNatan
Copy link
Contributor

LeoNatan commented Nov 7, 2020

That's how it was on iOS too, but according to feedback, it had to be in screen coords. CC @viktorijasujetaite

@noomorph
Copy link
Collaborator Author

noomorph commented Nov 7, 2020

@LeoNatan, thanks. I've been suspecting something like that. 😳
Ok, will rewrite the logic on Monday, apparently.

@LeoNatan
Copy link
Contributor

LeoNatan commented Nov 7, 2020

How was it before your changes? The Espresso swipe…?

@LeoNatan
Copy link
Contributor

LeoNatan commented Nov 7, 2020

If you need to write your own logic for the start and end points, you can copy the logic from the iOS side. The logic should be the same.

@d4vidi
Copy link
Collaborator

d4vidi commented Nov 8, 2020

@noomorph I agree with @LeoNatan - swiping has to start somewhere inside the view bounds, but end as farther away as the screen allows it.

@d4vidi
Copy link
Collaborator

d4vidi commented Nov 8, 2020

Well @noomorph I'm sort of done reviewing this, for now - seems there's a lot to go over for now, so let's reiterate this when the time is right

@noomorph
Copy link
Collaborator Author

@d4vidi , @LeoNatan

Now the implementations feel aligned, right?

@noomorph noomorph marked this pull request as ready for review November 12, 2020 20:27
Copy link
Collaborator

@d4vidi d4vidi left a comment

Choose a reason for hiding this comment

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

@noomorph amazing work overall! I hope you don't mind me nitpicking here and there 😂 Anyways it's mostly naming that bothered me in my way of understanding the code, so most comments are in that area. Other than that seem pretty good!

Copy link
Collaborator

@d4vidi d4vidi left a comment

Choose a reason for hiding this comment

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

Nice work! It's great to see how eventually you've used the generic algebric constructs to calculate both the absolute and normalized starting point, and better yet - the ending point.

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

Successfully merging this pull request may close these issues.

None yet

3 participants