[ViewPager] Add setPageWithoutAnimation #3621

brentvatne · 2015-10-23T00:07:03Z

In some cases it's desirable to set the page in the ViewPager without animating it -- we have this in ScrollView with scrollWithoutAnimationTo. This PR adds setPageWithoutAnimation on ViewPager.

cc @ide @kmagiera

facebook-github-bot · 2015-10-23T00:08:36Z

By analyzing the blame information on this pull request, we identified @mkonicek to be a potential reviewer.

mkonicek · 2015-10-23T18:29:48Z

Thanks Brent! Will take a look.

mkonicek · 2015-10-26T23:00:27Z

ReactAndroid/src/main/java/com/facebook/react/views/viewpager/ReactViewPagerManager.java

@@ -41,6 +41,11 @@ public void setSelectedPage(ReactViewPager view, int page) {
    view.setCurrentItemFromJs(page);
  }

+  @ReactProp(name = "animatePageTransition")
+  public void setAnimatePageTransition(ReactViewPager view, boolean animatedPageTransition) {


nit: animated -> animate

Going to get rid of this as per comment below :)

brentvatne · 2015-10-30T00:13:55Z

@mkonicek - updated!

facebook-github-bot · 2015-10-30T00:15:44Z

@brentvatne updated the pull request.

mkonicek · 2015-10-30T08:59:51Z

Oh sorry I meant:

// All page updates are instant
<ViewPager animateSetPage={false} selectedPage={this.state.page} ... />

// All page updates are animated
<ViewPager selectedPage={this.state.page} ... />

Do you think that API would make sense?

It's similar to how you set the bounces property on a ScrollView - it's a prop to control its visual behavior.

With the current solution there are two ways to set a page which seems a bit confusing to me. Looking at the API I would think: logically a ViewPager only has one selected page, why does it also have a selectedPageWithoutAnimation? If I update one does the other also change?

mkonicek · 2015-10-30T09:15:25Z

Is it sufficient for you to create a ViewPager that never animates, or do you need to flip between animated and instant page sets dynamically?

The dynamic case is harder: I think we should have some way to set both animateSetPage and selectedPage in the same batch (same render()) and have the native side set all the props first and only then call setCurrentItemFromJs, so it can determine whether to animate or not based on values of other props.

mkonicek · 2015-10-30T09:43:58Z

Just talked to Krzysztof who made a really good point - another option is not to have a selectedPage prop at all but have methods setPage, setPageWithoutAnimation. But then, is it the right API from React's perspective?

We think this requires more discussion, will chat about it with more people at fb to get more opinions.

brentvatne · 2015-10-30T16:07:56Z

Just talked to Krzysztof who made a really good point - another option is not to have a selectedPage prop at all but have methods setPage, setPageWithoutAnimation. But then, is it the right API from React's perspective?

This is the exact approach that we are currently using with this PR 😄 -- the public interface for props only exposes initialPage, onPageScroll, onPageSelected, and keyboardDismissMode. In addition to this, we expose setPage or setPageWithoutAnimation functions that can be called on the ref.

Internally, when setPage or setPageWithoutAnimation are called, we use props to update those. I think this part is an implementation detail and I just went along with how setPage was implemented already, but if we wanted some more consistency with how it's done on iOS at the moment then we could look at ScrollView and the equivalents scrollTo and scrollWithoutAnimationTo. These rely on bridge methods on RCTUIManager of the same name. So the implementation of setPage(selectedPage) on ViewPagerAndroid.android.js would look something like this:

  setPage: function(selectedPage) {
    ViewPagerManager.setPage(selectedPage);
  },

rather than its current implementation:

  setPage: function(selectedPage) {
    this.setState({
      selectedPage,
    });
  },

brentvatne · 2015-10-31T00:58:22Z

@mkonicek - ok thinking about this some more and I think the currently implemented approach of using props to update the page is flawed. Think of the following situation:

this.viewPager.setPage(5)
Manually scroll to page 1
this.viewPager.setPage(5)
Page remains on 1. This is because React sees currentPage as 5 on the ViewPager, so and when we setState via setPage we set it to 5 again, which of course results in no update being sent over to native.

So, I think for values that live on the native side like this and scroll position and such, we either need to have a very good approach for keeping that in sync with the JS side or just use the approach from ScrollView to set them imperatively.

machard · 2015-11-06T20:00:08Z

I need this feature too to make a looping carousel.
I looked quickly how to code the imperative approach but I'm not sure how to access the view object from a ReactMethod. I think I should use the reactTag but I'm not sure what to use to get the view from it.

mkonicek · 2015-11-07T13:40:10Z

@brentvatne I realized we also want to make the ViewPagerAndroid API closer to the iOS ScrollView for consistency: We might even want to render a ViewPagerAndroid under the hood if you specify <ScrollView pagingEnabled />.

Therefore I think we should get rid of the prop and have a function to do the paging imperatively, like ScrollView does.

We should make this work exactly the same way as on iOS. It doesn't look like ScrollView has an easy way to scroll to a given page though?

scrollTo: function(destY?: number, destX?: number) {
  // $FlowFixMe - Don't know how to pass Mixin correctly. Postpone for now
  this.getScrollResponder().scrollResponderScrollTo(destX || 0, destY || 0);
},

We should support that on both platforms.

We can make this consistent in steps:

First, change the ViewPagerAndroid API to have two imperative functions for scrolling, get rid of the prop. Update the example. That's a good start, let's do that in this PR.
Add support for scrolling to page to iOS ScrollView
Unify both APIs

mkonicek · 2015-11-07T13:47:46Z

@machard Appreciate your help! You can read the code for ScrollView.scrollTo for example, this will be very similar. But I think @brentvatne has got this once he's back from vacation :)

mkonicek · 2015-11-07T13:49:23Z

@brentvatne Your example with setting the page to 5, scrolling and then setting it again is great! Should have thought of it. Luckily it's not so hard to change it :)

facebook-github-bot · 2015-11-07T17:34:14Z

@brentvatne updated the pull request.

facebook-github-bot · 2015-11-07T17:40:19Z

@brentvatne updated the pull request.

brentvatne · 2015-11-07T17:44:01Z

Hiya @mkonicek!

@brentvatne I realized we also want to make the ViewPagerAndroid API closer to the iOS ScrollView for consistency: We might even want to render a ViewPagerAndroid under the hood if you specify .

Yup! That would be nice to have eventually.

Therefore I think we should get rid of the prop and have a function to do the paging imperatively, like ScrollView does.

We should make this work exactly the same way as on iOS. It doesn't look like ScrollView has an easy way to scroll to a given page though?

Correct! Right now you have to just know the width of each page and multiply that by the page that you want to be on. Would be nice to have this on both though, agreed.

We can make this consistent in steps:

First, change the ViewPagerAndroid API to have two imperative functions for scrolling, get rid of the prop. Update the example. That's a good start, let's do that in this PR.

Add support for scrolling to page to iOS ScrollView

Unify both APIs

Sounds good! I took care of #1 here. setPage and setPageWithoutAnimation are implemented imperatively now.

ide · 2015-11-07T22:09:56Z

A brief thought on paginated scrollviews and ViewPager -- we may want (hypothetically speaking) ViewPager.ios.js to use a paginated ScrollView instead of having ScrollView sometimes render a ViewPager. I think of ScrollView as a lower-level component than ViewPager, and it might be easier to implement ViewPager on iOS using a ScrollView, than it is to implement a paginated ScrollView on Android using a ViewPager (think of all the scroll responder APIs).

mkonicek · 2015-11-08T00:22:30Z

That's a good point @ide.

@brentvatne Looks awesome! I think you might be able to inline the ReactViewPagerCommandHelper into the View pager, the reason we have that helper for ScrollView is that there is a vertical and horizontal one and wanted to share code between them.

Does the initialPage prop as expected with this change? Could you also try running and update the example please?
https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/ViewPagerAndroidExample.android.js

…crollView

facebook-github-bot · 2015-11-08T12:45:14Z

@brentvatne updated the pull request.

brentvatne · 2015-11-08T12:45:52Z

I think you might be able to inline the ReactViewPagerCommandHelper into the View pager, the reason we have that helper for ScrollView is that there is a vertical and horizontal one and wanted to share code between them.

Done!

Does the initialPage prop as expected with this change? Could you also try running and update the example please?

It does now! Updated example to add a button to toggle between using setPage and setPageWithoutAnimation

brentvatne · 2015-11-08T12:46:59Z

Libraries/Components/ViewPager/ViewPagerAndroid.android.js

    return this.refs[VIEWPAGER_REF].getInnerViewNode();
  },

-  _childrenWithOverridenStyle: function() {
+  _childrenWithOverridenStyle: function(): Array {


Not great with flow -- what should I put here as the return type @mkonicek?

Good question. Try Array<ReactElement> and run flow from the root of the repo and see if that works.

Doesn't look like the React codebase uses Flow and not sure how the Flow handles the boundaries between code that uses Flow and code that doesn't.

If Array<ReactElement> doesn't work I'd go for Array or Array<Object>. It's a private function you're using just in this file.

It is no longer required to override styles this way here. We did this initially because we didn't have view flattening on android back then. Now that view flattening is there we should just wrap children in views positioned absolutely and with collapsable=false similarly to how we do it here: https://github.com/facebook/react-native/blob/master/Libraries/Components/ScrollView/RecyclerViewBackedScrollView.android.js#L95

@kmagiera It's a good point and looked into doing that when open sourcing the view pager but decided not to (don't remember the exact reason anymore). I think changing this is outside the scope of Brent's PR though. I'll pull this in as it looks good otherwise.

mkonicek · 2015-11-12T16:52:14Z

Looks awesome! 👍 Thanks for improving the example and using Flow too!

Sorry for the delay. Feel free to try out what return type works best for _childrenWithOverridenStyle (or just use Array) and ping me when you want this merged :)

mkonicek · 2015-11-13T14:57:07Z

@facebook-github-bot shipit

facebook-github-bot · 2015-11-13T14:59:26Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/568096366678113/int_phab to review.

Summary: In some cases it's desirable to set the page in the ViewPager without animating it -- we have this in ScrollView with `scrollWithoutAnimationTo`. This PR adds `setPageWithoutAnimation` on ViewPager. cc ide kmagiera Closes facebook#3621 Reviewed By: svcscm Differential Revision: D2652056 Pulled By: mkonicek fb-gh-sync-id: 6f1f38558c41ffdd863c0ebb2f046c75b5c0392c

Summary: In some cases it's desirable to set the page in the ViewPager without animating it -- we have this in ScrollView with `scrollWithoutAnimationTo`. This PR adds `setPageWithoutAnimation` on ViewPager. cc ide kmagiera Closes facebook/react-native#3621 Reviewed By: svcscm Differential Revision: D2652056 Pulled By: mkonicek fb-gh-sync-id: 6f1f38558c41ffdd863c0ebb2f046c75b5c0392c

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 23, 2015

brentvatne assigned mkonicek Oct 25, 2015

mkonicek reviewed Oct 26, 2015
View reviewed changes

machard referenced this pull request in machard/react-native Nov 6, 2015

view pager allows setPage without animation

c5d3ab2

machard referenced this pull request in machard/react-native Nov 6, 2015

view pager allows setPage without animation

7fe9e78

brentvatne added 3 commits November 8, 2015 11:10

[ViewPager] Add setPageWithoutAnimation and make it imperative like S…

7a8e206

…crollView

[ViewPager] Make initialPage work again and update the example

455ed92

[ViewPager] Inline receiveCommand and getCommandsMap

ce19b2d

brentvatne reviewed Nov 8, 2015
View reviewed changes

brentvatne closed this in 50b8b00 Nov 13, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ViewPager] Add setPageWithoutAnimation #3621

[ViewPager] Add setPageWithoutAnimation #3621

brentvatne commented Oct 23, 2015

facebook-github-bot commented Oct 23, 2015

mkonicek commented Oct 23, 2015

mkonicek Oct 26, 2015

brentvatne Oct 29, 2015

brentvatne commented Oct 30, 2015

facebook-github-bot commented Oct 30, 2015

mkonicek commented Oct 30, 2015

mkonicek commented Oct 30, 2015

mkonicek commented Oct 30, 2015

brentvatne commented Oct 30, 2015

brentvatne commented Oct 31, 2015

machard commented Nov 6, 2015

mkonicek commented Nov 7, 2015

mkonicek commented Nov 7, 2015

mkonicek commented Nov 7, 2015

facebook-github-bot commented Nov 7, 2015

facebook-github-bot commented Nov 7, 2015

brentvatne commented Nov 7, 2015

ide commented Nov 7, 2015

mkonicek commented Nov 8, 2015

facebook-github-bot commented Nov 8, 2015

brentvatne commented Nov 8, 2015

brentvatne Nov 8, 2015

mkonicek Nov 12, 2015

kmagiera Nov 13, 2015

mkonicek Nov 13, 2015

mkonicek commented Nov 12, 2015

mkonicek commented Nov 13, 2015

facebook-github-bot commented Nov 13, 2015

[ViewPager] Add setPageWithoutAnimation #3621

[ViewPager] Add setPageWithoutAnimation #3621

Conversation

brentvatne commented Oct 23, 2015

facebook-github-bot commented Oct 23, 2015

mkonicek commented Oct 23, 2015

mkonicek Oct 26, 2015

Choose a reason for hiding this comment

brentvatne Oct 29, 2015

Choose a reason for hiding this comment

brentvatne commented Oct 30, 2015

facebook-github-bot commented Oct 30, 2015

mkonicek commented Oct 30, 2015

mkonicek commented Oct 30, 2015

mkonicek commented Oct 30, 2015

brentvatne commented Oct 30, 2015

brentvatne commented Oct 31, 2015

machard commented Nov 6, 2015

mkonicek commented Nov 7, 2015

mkonicek commented Nov 7, 2015

mkonicek commented Nov 7, 2015

facebook-github-bot commented Nov 7, 2015

facebook-github-bot commented Nov 7, 2015

brentvatne commented Nov 7, 2015

ide commented Nov 7, 2015

mkonicek commented Nov 8, 2015

facebook-github-bot commented Nov 8, 2015

brentvatne commented Nov 8, 2015

brentvatne Nov 8, 2015

Choose a reason for hiding this comment

mkonicek Nov 12, 2015

Choose a reason for hiding this comment

kmagiera Nov 13, 2015

Choose a reason for hiding this comment

mkonicek Nov 13, 2015

Choose a reason for hiding this comment

mkonicek commented Nov 12, 2015

mkonicek commented Nov 13, 2015

facebook-github-bot commented Nov 13, 2015