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 ViewPager2 swipe support #368

Merged

Conversation

daniellAlgar
Copy link
Contributor

Hello!

This PR will add swipe support for ViewPager2.

It will use the exact same API as the current swipeViewPagerForward() and
swipeViewPagerBack() but will automatically use the available ViewPager/ViewPager2.

I ended up with a few nested try/catch which I'm not proud of. Please give input on it if you have a better solution. But I did see that you already have this pattern in the code base (e.g. BaristaListAssertions) so I thought I'd give it a try.

Best regards,
Daniell Algar

@daniellAlgar
Copy link
Contributor Author

Pinging some of the maintainers to see that you at least have noticed this PR
@Sloy @rocboronat @alorma

@Sloy
Copy link
Member

Sloy commented Sep 22, 2020

Hi @daniellAlgar! Thanks for the PR and for the ping! I saw the notification but it got lost in my inbox, sorry 😭

The approach you mentioned is fine by me if there's no other easy option. Let me take a look at the code to make sure there's nothing too weird :)

@Sloy Sloy requested a review from a team September 22, 2020 08:59
@daniellAlgar
Copy link
Contributor Author

Thank you @Sloy . There's no rush! I just wanted to make sure it wasn't getting forgotten :)

@@ -43,6 +55,12 @@ object SwipeActions {
NormalizedLocation(from.first, from.second),
NormalizedLocation(to.first, to.second),
Press.FINGER))

@JvmStatic
fun swipeViewPager2Forward(): ViewAction = ViewPager2SwipeAction(direction = FORWARD)
Copy link
Member

Choose a reason for hiding this comment

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

just thinking...

Should we add viewpager2 as part of viewpager swipe? aas other methods on barista do for list and recyclers for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. It's just that swiping a ViewPager2 is a bit more complicated than swiping the old ViewPager, since ViewPager2 supports both horizontal/vertical directions (plus we always have "right-to-left" layout to consider).

So if I were to add ViewPager2 directly in SwipeActions.swipe(from: Pair<..>, to: Pair<..>) I would have to figure out the pairs even before getting to this call. And that would make it even worse than what I have done today I think 🤷‍♂️
Now it is still the case that the user only do swipeViewPagerForward/Back() regardless of which view pager is being used.

But maybe I misunderstood your comment or I haven't thought this through enough :)

Copy link
Member

Choose a reason for hiding this comment

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

Good. Thanks for the explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just add this even if it might be obvious. But a "swipe forward" means different things in ViewPager2 depending on the horizontal/vertical orientation of it. That's what I was trying to say above :)

assertDisplayed("1")
}

/** Helper function */
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the comment. But it's not an stopper to approve the PR, of course 😄

@rocboronat
Copy link
Member

It looks great to me. Thanks for the contribution, @daniellAlgar ! 👏

@rocboronat
Copy link
Member

@alorma @Sloy time to merge?

@alorma
Copy link
Member

alorma commented Sep 30, 2020

Yes for me @rocboronat . Go for it

@Sloy
Copy link
Member

Sloy commented Sep 30, 2020

Yes!!

@rocboronat rocboronat merged commit b5c0ac7 into AdevintaSpain:master Sep 30, 2020
@rocboronat
Copy link
Member

🎉

@daniellAlgar
Copy link
Contributor Author

Thank you all :)

@alorma
Copy link
Member

alorma commented Oct 1, 2020

Released on 3.7.0 release

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

4 participants