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

Definable distance pagination for ScrollView #1532

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
@rxb
Contributor

rxb commented Jun 6, 2015

This is an enhancement for ScrollView that adds the ability to paginate based on a width other than the width of the ScrollView itself. This is a fairly common pattern used on apps like Facebook, App Store, and Twitter to scroll through a horizontal set of cards or icons:

img_8726 2 img_8727 2 img_8728 2

After trying to accomplish this only with JS, it appears that attempting to take over an in-progress native scroll animation with JS is always going to result in some amount of jankiness and jumping.

This pull request uses scrollViewWillEndDragging in RCTScrollView.m to adjust targetContentOffset based on two new optional props added to ScrollView. snapToInterval sets the multiple that the ScrollView will come to rest on. snapToAlignment sets the relative alignment of the snap start, center, or end.

Here's an example of it in action:
https://vid.me/aoby

Here is some sample code to try it out yourself:

var SnapTest = React.createClass({

  render: function() {
    var blocks = [];
    for(var i=0; i<=30; i++){
      blocks.push("i");
    }
    var blockW = 100;
    return (
      <ScrollView 
        horizontal={true} 
        decelerationRate={0}
        snapToInterval={blockW}
        snapToAlignment="start"
        >
        {blocks.map(function(m,i){
          return (
            <View key={i} style={{width: blockW, height: blockW, padding:16, backgroundColor: (i%2==0)?'cornflowerblue':'royalblue', borderWidth: 1, borderColor: "#fff"}}>
              <Text style={{color: 'white', fontWeight: '800'}}>{i}</Text>
            </View>
          );
        })}
      </ScrollView>
    );
  }
});

Would love to get your thoughts on this direction!

cc: @brentvatne #1362

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 6, 2015

Collaborator

This looks great, I'll read over more carefully and try it out tonight 😄 edit: scratch that, went on massive run today and am totally exhausted, tomorrow 👍

Collaborator

brentvatne commented Jun 6, 2015

This looks great, I'll read over more carefully and try it out tonight 😄 edit: scratch that, went on massive run today and am totally exhausted, tomorrow 👍

Show outdated Hide outdated React/Views/RCTScrollView.m
// What is the current offset?
CGFloat targetContentOffsetAlongAxis = isHorizontal ? targetContentOffset->x : targetContentOffset->y;
//CGFloat targetContentOffsetAlongAxis = isHorizontal ? scrollView.contentOffset.x : scrollView.contentOffset.y;

This comment has been minimized.

@brentvatne

brentvatne Jun 8, 2015

Collaborator

This comment can go

@brentvatne

brentvatne Jun 8, 2015

Collaborator

This comment can go

This comment has been minimized.

@rxb

rxb Jun 8, 2015

Contributor

fixed

@rxb

rxb Jun 8, 2015

Contributor

fixed

CGFloat snapToIntervalF = (CGFloat)self.snapToInterval;
// Find which axis to snap
BOOL isHorizontal = (scrollView.contentSize.width > self.frame.size.width);

This comment has been minimized.

@brentvatne

brentvatne Jun 8, 2015

Collaborator

Is this a safe way to determine whether or not it is horizontal? Could it ever occur that our primary scroll axis is vertical but the contentSize is both greater than the width and height of the scrollView?

@brentvatne

brentvatne Jun 8, 2015

Collaborator

Is this a safe way to determine whether or not it is horizontal? Could it ever occur that our primary scroll axis is vertical but the contentSize is both greater than the width and height of the scrollView?

This comment has been minimized.

@rxb

rxb Jun 8, 2015

Contributor

Was a little unsure about that one... I borrowed it from line 411

- (void)setStickyHeaderIndices:(NSIndexSet *)headerIndices
{
  RCTAssert(_scrollView.contentSize.width <= self.frame.size.width,
           @"sticky headers are not supported with horizontal scrolled views");
  _scrollView.stickyHeaderIndices = headerIndices;
}
@rxb

rxb Jun 8, 2015

Contributor

Was a little unsure about that one... I borrowed it from line 411

- (void)setStickyHeaderIndices:(NSIndexSet *)headerIndices
{
  RCTAssert(_scrollView.contentSize.width <= self.frame.size.width,
           @"sticky headers are not supported with horizontal scrolled views");
  _scrollView.stickyHeaderIndices = headerIndices;
}

This comment has been minimized.

@sahrens

sahrens Jun 13, 2015

Contributor

I think this is fairly safe, although things might get weird in a 2-axis scrolling situation...the JS component has a horizontal prop - maybe we should pass that through to native?

@sahrens

sahrens Jun 13, 2015

Contributor

I think this is fairly safe, although things might get weird in a 2-axis scrolling situation...the JS component has a horizontal prop - maybe we should pass that through to native?

Show outdated Hide outdated React/Views/RCTScrollView.m
if([self.snapToAlignment isEqualToString: @"center"]){
alignmentOffset = (frameLength * 0.5f) + (snapToIntervalF * 0.5f);
}
else if ([self.snapToAlignment isEqualToString: @"end"]) {

This comment has been minimized.

@brentvatne

brentvatne Jun 8, 2015

Collaborator

nit: this should be up on the same line as }

@brentvatne

brentvatne Jun 8, 2015

Collaborator

nit: this should be up on the same line as }

This comment has been minimized.

@rxb

rxb Jun 8, 2015

Contributor

fixed

@rxb

rxb Jun 8, 2015

Contributor

fixed

Show outdated Hide outdated React/Views/RCTScrollView.m
if(isHorizontal) {
targetContentOffset->x = newTargetContentOffset;
}
else {

This comment has been minimized.

@brentvatne

brentvatne Jun 8, 2015

Collaborator

nit: this should be up on the same line as }

@brentvatne

brentvatne Jun 8, 2015

Collaborator

nit: this should be up on the same line as }

This comment has been minimized.

@rxb

rxb Jun 8, 2015

Contributor

fixed

@rxb

rxb Jun 8, 2015

Contributor

fixed

Show outdated Hide outdated React/Views/RCTScrollView.m
// Pick snap point based on direction and proximity
NSInteger snapIndex = floor((targetContentOffsetAlongAxis + alignmentOffset) / snapToIntervalF);
if(translationAlongAxis < 0) {

This comment has been minimized.

@brentvatne

brentvatne Jun 8, 2015

Collaborator

this is more would be more concise with a ternary expression

@brentvatne

brentvatne Jun 8, 2015

Collaborator

this is more would be more concise with a ternary expression

This comment has been minimized.

@rxb

rxb Jun 8, 2015

Contributor

fixed

@rxb

rxb Jun 8, 2015

Contributor

fixed

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 8, 2015

Collaborator

@rxb - could you also squash these commits into one?

Collaborator

brentvatne commented Jun 8, 2015

@rxb - could you also squash these commits into one?

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 8, 2015

Collaborator

Not sure who owns ScrollView cc @tadeuzagallo @nicklockwood

Collaborator

brentvatne commented Jun 8, 2015

Not sure who owns ScrollView cc @tadeuzagallo @nicklockwood

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Jun 8, 2015

Contributor

@brentvatne Thanks for the notes. Everything except for the isHorizontal question is fixed and squashed into 1 commit.

Contributor

rxb commented Jun 8, 2015

@brentvatne Thanks for the notes. Everything except for the isHorizontal question is fixed and squashed into 1 commit.

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Jun 11, 2015

Contributor

What do you think about tweaking the api to match up with the CSS scroll-snap-points spec?
http://www.w3.org/TR/css-snappoints-1/#scroll-snap-type

Looks like it will be in most browsers soon:
http://caniuse.com/#feat=css-snappoints
https://www.chromestatus.com/features/5721832506261504

Contributor

rxb commented Jun 11, 2015

What do you think about tweaking the api to match up with the CSS scroll-snap-points spec?
http://www.w3.org/TR/css-snappoints-1/#scroll-snap-type

Looks like it will be in most browsers soon:
http://caniuse.com/#feat=css-snappoints
https://www.chromestatus.com/features/5721832506261504

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 11, 2015

Collaborator

@rxb - Absolutely! I think the closer that we can be to the w3 spec the better. Great point, can you adjust accordingly? cc @vjeux

Collaborator

brentvatne commented Jun 11, 2015

@rxb - Absolutely! I think the closer that we can be to the w3 spec the better. Great point, can you adjust accordingly? cc @vjeux

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jun 11, 2015

Contributor

Have you tried paginate={true} on ScrollView, doesn't it solve your use case?

Contributor

vjeux commented Jun 11, 2015

Have you tried paginate={true} on ScrollView, doesn't it solve your use case?

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 11, 2015

Collaborator

@vjeux - there are several problems with paginate={true}:

  • Fixed page size to the viewport width
    • If it were to support variable widths, it would need to have an option to specify where to snap to (middle, left-side, right-side)
  • Horizontal paging only

Like in the screenshots that @rxb posted in the original comment above, it's quite common to want to snap to something other than the viewport width

Collaborator

brentvatne commented Jun 11, 2015

@vjeux - there are several problems with paginate={true}:

  • Fixed page size to the viewport width
    • If it were to support variable widths, it would need to have an option to specify where to snap to (middle, left-side, right-side)
  • Horizontal paging only

Like in the screenshots that @rxb posted in the original comment above, it's quite common to want to snap to something other than the viewport width

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 11, 2015

Collaborator

You can support variable widths with pagination with a couple of tricks -- that is how the Apple Photos app works.

The core difference between pagination and scroll snapping is that pagination snaps to the next page even if you swipe hard, while scroll snapping accounts for momentum so you could end up several pages away.

Collaborator

ide commented Jun 11, 2015

You can support variable widths with pagination with a couple of tricks -- that is how the Apple Photos app works.

The core difference between pagination and scroll snapping is that pagination snaps to the next page even if you swipe hard, while scroll snapping accounts for momentum so you could end up several pages away.

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Jun 11, 2015

Contributor

I think where that difference matters most is when you are scrolling through items that are not close to the screen width, like photos on FB profiles:
https://vid.me/V4Bz

It would be rough if it forced one-at-a-time, but free scrolling would feel like driving on a greased track.

Contributor

rxb commented Jun 11, 2015

I think where that difference matters most is when you are scrolling through items that are not close to the screen width, like photos on FB profiles:
https://vid.me/V4Bz

It would be rough if it forced one-at-a-time, but free scrolling would feel like driving on a greased track.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jun 13, 2015

Contributor

@nicklockwood: what do you think of this?

Contributor

sahrens commented Jun 13, 2015

@nicklockwood: what do you think of this?

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Jun 17, 2015

Contributor

@brentvatne Awesome, I'll sync things up with the W3C spec.

After testing this in a more realistic screen -- where the horizontal scrolling section is one part of a longer vertical scrollview -- ran into that nested scrolling bug from: #41 .

Contributor

rxb commented Jun 17, 2015

@brentvatne Awesome, I'll sync things up with the W3C spec.

After testing this in a more realistic screen -- where the horizontal scrolling section is one part of a longer vertical scrollview -- ran into that nested scrolling bug from: #41 .

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jul 15, 2015

Contributor

Just wanted to said that I patched this to prototype a use case that we wanted to do internally and it works really well :)

Sorry it takes so long to review :(

Contributor

vjeux commented Jul 15, 2015

Just wanted to said that I patched this to prototype a use case that we wanted to do internally and it works really well :)

Sorry it takes so long to review :(

Show outdated Hide outdated React/Views/RCTScrollView.m
}
}
<<<<<<< HEAD

This comment has been minimized.

@paramaggarwal

paramaggarwal Jul 15, 2015

Contributor

merge conflict artifact?

@paramaggarwal

paramaggarwal Jul 15, 2015

Contributor

merge conflict artifact?

This comment has been minimized.

@rxb

rxb Jul 15, 2015

Contributor

Thanks for the catch, fixed.

@rxb

rxb Jul 15, 2015

Contributor

Thanks for the catch, fixed.

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Jul 15, 2015

Contributor

@vjeux Happy to hear it worked for you.

I keep meaning to get back to this and tweak to match the W3C scroll snap points spec. Would love to get anyone's thoughts on this interface:

Enable snapping
http://www.w3.org/TR/css-snappoints-1/#scroll-snap-type
prop: scrollSnapType
values: none proximity (mandatory could be supported in the future )

Set snapping interval
http://www.w3.org/TR/css-snappoints-1/#propdef-scroll-snap-points-x
prop: scrollSnapPoints (W3C spec defines scroll-snap-points-x and scroll-snap-points-y independently, but it seems that RN encourages scrolling in one direction at a time)
values: none repeat([interval in pixels]) (The repeat seems unnecessary, but it is part of the spec.)

Set alignment of snap relative to scroll container
http://www.w3.org/TR/css-snappoints-1/#propdef-scroll-snap-destination
prop: scrollSnapDestination
values: [offset in pixels] (Spec defines values for x and y offsets together, but this seems unnecessary if RN ScrollViews are only scrolling in one direction. Setting this explicitly in pixels gives more flexibility, but is more work than start, center, end )

Contributor

rxb commented Jul 15, 2015

@vjeux Happy to hear it worked for you.

I keep meaning to get back to this and tweak to match the W3C scroll snap points spec. Would love to get anyone's thoughts on this interface:

Enable snapping
http://www.w3.org/TR/css-snappoints-1/#scroll-snap-type
prop: scrollSnapType
values: none proximity (mandatory could be supported in the future )

Set snapping interval
http://www.w3.org/TR/css-snappoints-1/#propdef-scroll-snap-points-x
prop: scrollSnapPoints (W3C spec defines scroll-snap-points-x and scroll-snap-points-y independently, but it seems that RN encourages scrolling in one direction at a time)
values: none repeat([interval in pixels]) (The repeat seems unnecessary, but it is part of the spec.)

Set alignment of snap relative to scroll container
http://www.w3.org/TR/css-snappoints-1/#propdef-scroll-snap-destination
prop: scrollSnapDestination
values: [offset in pixels] (Spec defines values for x and y offsets together, but this seems unnecessary if RN ScrollViews are only scrolling in one direction. Setting this explicitly in pixels gives more flexibility, but is more work than start, center, end )

@sahrens

This comment has been minimized.

Show comment
Hide comment
Contributor

sahrens commented Jul 15, 2015

@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Jul 24, 2015

Collaborator

+1

Just stumbled upon this while looking to detect page changes on a horizontal listview. This could be useful i guess.

Collaborator

chirag04 commented Jul 24, 2015

+1

Just stumbled upon this while looking to detect page changes on a horizontal listview. This could be useful i guess.

@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Jul 24, 2015

Collaborator

@vjeux Does it make sense to emulate css api if it is not part of style?

My 2cents: I liked the initial api suggested by @rxb.

Collaborator

chirag04 commented Jul 24, 2015

@vjeux Does it make sense to emulate css api if it is not part of style?

My 2cents: I liked the initial api suggested by @rxb.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jul 24, 2015

Contributor

The guideline is to first try to design the best API based on what exists on iOS, Android, web... Then once we found one, if it is close enough to the web one, it's preferable to use it. If the web one doesn't make sense, we shouldn't tie ourself to it

Contributor

vjeux commented Jul 24, 2015

The guideline is to first try to design the best API based on what exists on iOS, Android, web... Then once we found one, if it is close enough to the web one, it's preferable to use it. If the web one doesn't make sense, we shouldn't tie ourself to it

@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Jul 28, 2015

Collaborator

any thoughts one this one?

cc @nicklockwood

Collaborator

chirag04 commented Jul 28, 2015

any thoughts one this one?

cc @nicklockwood

@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Aug 2, 2015

Collaborator

@vjeux Mind sharing the patch? Wondering if anyone is having any opinions on this. Happy to implement thoughts that others may have.

Collaborator

chirag04 commented Aug 2, 2015

@vjeux Mind sharing the patch? Wondering if anyone is having any opinions on this. Happy to implement thoughts that others may have.

@syrusakbary

This comment has been minimized.

Show comment
Hide comment
@syrusakbary

syrusakbary Aug 10, 2015

Would love having this patch merged into react native!
👍

Would love having this patch merged into react native!
👍

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Sep 15, 2015

Contributor

If there's anything I can do to help get this moving, let me know! I've been using in my personal project and it's been pretty useful.

Contributor

rxb commented Sep 15, 2015

If there's anything I can do to help get this moving, let me know! I've been using in my personal project and it's been pretty useful.

@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Sep 15, 2015

Collaborator

@rxb Def want to see this landed and want to use it in our project as well. It is still compatible with RN 0.11?

@vjeux @nicklockwood Can we land this one?

Collaborator

chirag04 commented Sep 15, 2015

@rxb Def want to see this landed and want to use it in our project as well. It is still compatible with RN 0.11?

@vjeux @nicklockwood Can we land this one?

@vjeux

This comment has been minimized.

Show comment
Hide comment
Contributor

vjeux commented Sep 15, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Sep 15, 2015

Collaborator

Awesome. Thanks 👍

Lrn flashcards will use this ;)

Collaborator

chirag04 commented Sep 15, 2015

Awesome. Thanks 👍

Lrn flashcards will use this ;)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Sep 15, 2015

Contributor

"Failed to apply patch". @rxb would you be willing to rebase? Sorry for the wait

Contributor

vjeux commented Sep 15, 2015

"Failed to apply patch". @rxb would you be willing to rebase? Sorry for the wait

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Sep 15, 2015

Contributor

Should be back in business, rebased with master. The Travis e2e test is failing, but I can't tell if that's because of something in this PR or a general thing that's happening. Seems like almost all the recent PRs are failing that.

Contributor

rxb commented Sep 15, 2015

Should be back in business, rebased with master. The Travis e2e test is failing, but I can't tell if that's because of something in this PR or a general thing that's happening. Seems like almost all the recent PRs are failing that.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Sep 15, 2015

Contributor

Travis is broken because of our android push. Unlikely your fault

Contributor

vjeux commented Sep 15, 2015

Travis is broken because of our android push. Unlikely your fault

miracle2k added a commit to miracle2k/react-native that referenced this pull request Sep 16, 2015

miracle2k added a commit to miracle2k/react-native that referenced this pull request Sep 16, 2015

@vjeux

This comment has been minimized.

Show comment
Hide comment
Contributor

vjeux commented Sep 22, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@ghost ghost closed this in dcf245a Sep 23, 2015

@rxb

This comment has been minimized.

Show comment
Hide comment
@rxb

rxb Sep 23, 2015

Contributor

Thanks, everybody!

Contributor

rxb commented Sep 23, 2015

Thanks, everybody!

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Sep 23, 2015

Contributor

Thanks for sticking with us and sorry it took that much time!

Contributor

vjeux commented Sep 23, 2015

Thanks for sticking with us and sorry it took that much time!

MattFoley added a commit to skillz/react-native that referenced this pull request Nov 9, 2015

Definable distance pagination for ScrollView
Summary: This is an enhancement for ScrollView that adds the ability to paginate based on a width other than the width of the ScrollView itself. This is a fairly common pattern used on apps like Facebook, App Store, and Twitter to scroll through a horizontal set of cards or icons:

![img_8726 2](https://cloud.githubusercontent.com/assets/451050/8017899/39f9f47c-0bd2-11e5-9c1d-889452f20cf7.PNG) ![img_8727 2](https://cloud.githubusercontent.com/assets/451050/8017898/39f962dc-0bd2-11e5-98b4-461ac0f7f21b.PNG)  ![img_8728 2](https://cloud.githubusercontent.com/assets/451050/8017900/39fd91a4-0bd2-11e5-8786-4cf0316295a0.PNG)

After trying to accomplish this only with JS, it appears that attempting to take over an in-progress native scroll animation with JS is always going to result in some amount of jankiness and jumping.

This pull request uses `scrollViewWillEndDragging` in RCTScrollView.m to adjust `targetContentOffset` based on two new optional props added to ScrollView. `snapToInterval` sets the multiple that the
Closes facebook#1532

Reviewed By: @​svcscm, @​trunkagent

Differential Revision: D2443701

Pulled By: @vjeux
@aleclarson

This comment has been minimized.

Show comment
Hide comment
@aleclarson

aleclarson Jan 7, 2016

Contributor

I'm surprised an onSnap prop wasn't included. Thoughts?

Contributor

aleclarson commented Jan 7, 2016

I'm surprised an onSnap prop wasn't included. Thoughts?

@erickreutz

This comment has been minimized.

Show comment
Hide comment
@erickreutz

erickreutz Aug 10, 2016

Contributor

Any plans to bring this functionality to Android?

Contributor

erickreutz commented Aug 10, 2016

Any plans to bring this functionality to Android?

@me-abhinav

This comment has been minimized.

Show comment
Hide comment
@me-abhinav

me-abhinav Oct 5, 2016

+1 for Android

+1 for Android

@me-abhinav

This comment has been minimized.

Show comment
Hide comment
@me-abhinav

me-abhinav Oct 6, 2016

I can create a pull request for Android if the owner wants.

me-abhinav commented Oct 6, 2016

I can create a pull request for Android if the owner wants.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment