-
Notifications
You must be signed in to change notification settings - Fork 744
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 precise swipe actions for swipes from a start to an end point #675
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
EarlGrey/Action/GREYActions.h
Outdated
* Returns an action that swipes through the view from a given @c startPoint to a given | ||
* @c endPoint. | ||
* | ||
* @param startPoint The point where the swipe should begin. |
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.
Is the startpoint based on a point in the whole screen or within the matched view?
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.
It is based on the whole screen currently.
But it might make more sense to base this on the origin of the matched view.
I will change that and clarify in the documentation.
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.
@steviki any updates on this?
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.
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.
QQ - isn't this the same as scroll amount? Ir are you trying to do from any point to another?
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.
It doesn't seem to be the same as scrolling in a direction with a given amount, as the scroll amount action only allows scrolling in a GREYDirection
(which is restricted to left, right, up, and down) and not any arbitrary direction. While scrolling from a specific point to another point allows to scroll in any direction without restriction.
Just wanted to check in on the status here. |
It looks like you want precision here. Is there a reason for not using scroll API which is much more precise than swipe? |
@khandpur I'm not quite sure if I understand your question correctly, so please correct me if I'm wrong. |
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.
Could you add some tests for the new action?
@khandpur Sorry for the long delay, but I've now added unit and functional tests for the precise swipe actions. |
@khandpur Just wanted to ping to check on the status of this PR. I would really appreciated if you could have another look at this PR and consider merging it. Let me know if there are any questions! |
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.
Mostly minor nits. Would be great if you could wrap header files with Nullability macros (NS_ASSUME_NONNULL_BEGIN / END) as well. Once done, I'll merge the changes. Thanks!
bdc102f
to
edd869e
Compare
Thanks for the review @khandpur. |
@tirodkar PTAL and merge if it looks good. |
And thank you Stefan for adding this feature request! |
50b518c
to
51ae4fc
Compare
Thank you for reviewing this PR and getting it merged! I really appreciate it. @tirodkar I also addressed your review feedback now and fixed the typo. |
@tirodkar I've restarted the CI. Could we merge this and make it available in earlgrey 2? |
Hi! Are there any blockers left for merging this? |
This PR implements and fixes #674.
Introduces new swipe actions, that take a
starPoint
and anendPoint
and perform a swipe along those points via two new public functions:grey_swipeFastFromStartToEndPoint(CGPoint startPoint, CGPoint endPoint);
grey_swipeSlowFromStartToEndPoint(CGPoint startPoint, CGPoint endPoint);
This is done via the new
GREYPreciseSwipeAction
class, that is implemented similar toGREYSwipeAction
, but instead of taking adirection
,duration
andstartPercents
, the precise swipe action takes astartPoint
,endPoint
andduration
.When performing this action, a swipe with a single touch is executed that follows the path from the
startPoint
to theendPoint
in the time of the givenduration
.