Skip to content

[Idea: ScrollView] Add getScrollResponder to ScrollView for composition - #766

Closed
ide wants to merge 1 commit into
react:masterfrom
ide:listview-api-2
Closed

[Idea: ScrollView] Add getScrollResponder to ScrollView for composition#766
ide wants to merge 1 commit into
react:masterfrom
ide:listview-api-2

Conversation

@ide

@ideide commented Apr 9, 2015

Copy link
Copy Markdown
Contributor

This is a proposal to add getScrollResponder to all ScrollView-like components, including ListView. This allows multiple higher-order scroll views to be composed while allowing the owner of the top-level scroll view to call scrollableView.getScrollResponder().scrollTo(...) regardless of whether scrollableView is a ScrollView, ListView, InvertedScrollView, etc.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 9, 2015
@nicklockwood

Copy link
Copy Markdown
Contributor

I think I need a little more context about how this is used. Do you have an example implementation you can point me to?

@ide

ide commented Apr 9, 2015

Copy link
Copy Markdown
ContributorAuthor

I'll start with some background to paint a complete picture: I'm building "higher-order" scroll views that have some extra functionality. For example, an infinite scroll view invokes a callback to fetch more data when the scroll position approaches the end. This could look like:

classInfiniteScrollViewextendsReact.Component{scrollTo(destY,destX){this.refs.scrollView.scrollTo(destY,destX);}render(){return(<ScrollView{...this.props}ref="scrollView"onScroll={(event)=>{if(this._shouldLoadMoreContent(event)){this.props.onLoadMore();}}}>{this.props.children}</ScrollView>);}}// * this could be a mixin, but there are other advanced scroll views that require// a new component class

Now I'd like to use a different kind of scroll view, like a ListView, ReversedScrollView, etc. so I add a prop like this.props.scrollViewClass and use it to create the component returned from render(). This works well and so far I'm happy with the flexibility from this style of composition.

The one snag is implementing methods like getInnerViewNode, scrollTo, and scrollWithoutAnimationTo. In the core classes, these methods are implemented only on ScrollView, while ListView defines getScrollResponder() which returns the underlying ScrollView. So I came up with two approaches:

  1. Implement getScrollResponder() on every ScrollView-like component class. This lets you get the underlying ScrollView and then you can call scrollTo, etc. on it. By adding this method to ScrollView itself, you can indiscriminately call scrollComponent.getScrollResponder().scrollTo(0, 0) regardless of the actual type of the scrollable component. This PR implements this approach.
  2. Implement scrollTo, scrollWithoutAnimationTo, and getInnerViewNode on every ScrollView-like component class. This makes it convenient to call scrollComponent.scrollTo(0, 0) regardless of the scroll component's type. This is implemented in [Image] Update Image docs to include isStatic flag #755.

I prefer the first approach because if ScrollView's methods are added/removed/renamed, the code for all other scrollable components doesn't need to be updated. It's a little more verbose with the getScrollResponder() call but I believe that can be papered over with a mixin.

ScrollableMixin={scrollTo(destY,destX){invariant(this.getScrollResponder,'Scrollable components must implement getScrollResponder');this.getScrollResponder().scrollTo(destY,destX);}};

@nicklockwood

Copy link
Copy Markdown
Contributor

Thank you for the detailed explanation. That makes a lot of sense to me. @vjeux?

@vjeux

Copy link
Copy Markdown
Contributor

I add a prop like this.props.scrollViewClass and use it to create the component returned from render()

I've been meaning to do something similar for a while in order to support both iOS and Android specific list views but a single ListView/InfiniteScrollView/RefreshableScrollView...

We've been moving away from passing a class as props to passing a function. This way the call site can pass props and the implementation can cloneElement to pass props as well.

<InfiniteScrollView renderScrollView={() => <ScrollView style={{paddingTop: 50}} />} />

@ide

ide commented Apr 11, 2015

Copy link
Copy Markdown
ContributorAuthor

@vjeux good idea with the function. Just wondering, is there a reason the prop takes a function instead of a component instance directly? Maybe how the ref owner is determined?

@vjeux

Copy link
Copy Markdown
Contributor

More about future proofing the api, i can almost guarantee you that you're going to want to pass arguments computed from within the extended scroll view component at some point

@ide

ide commented Apr 13, 2015

Copy link
Copy Markdown
ContributorAuthor

Thought about it some more - the function also lets us do nested composition e.g.

<ScrollViewArenderScrollView={()=><ScrollViewBrenderScrollView={()=><ScrollViewC/>}/>}/>

@ide
ideforce-pushed the listview-api-2 branch 3 times, most recently from 6286480 to bf524eeCompareApril 15, 2015 20:59
@ide
ideforce-pushed the listview-api-2 branch 2 times, most recently from bc49683 to a02056fCompareApril 28, 2015 01:24
This is a proposal to add `getScrollResponder` to all ScrollView-like components, including ListView. This allows multiple higher-order scroll views to be composed while allowing the owner of the top-level scroll view to call `scrollableView.getScrollResponder().scrollTo(...)` regardless of whether `scrollableView` is a ScrollView, ListView, InvertedScrollView, etc.
@ide
ideforce-pushed the listview-api-2 branch from 9dfaaca to fd2ba8bCompareMay 8, 2015 11:34
@brentvatnebrentvatne changed the title [RFC][ScrollView] Add getScrollResponder to ScrollView for composition[Idea: ScrollView] Add getScrollResponder to ScrollView for compositionJun 1, 2015
@sahrenssahrens assigned frantic and sahrens and unassigned franticJun 11, 2015
@sahrens

Copy link
Copy Markdown
Contributor

Oops, wrong PR.

@sahrens

Copy link
Copy Markdown
Contributor

@facebook-github-bot import

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

sahrens pushed a commit to sahrens/react-native that referenced this pull request Jun 12, 2015
…tion
Summary:
This is a proposal to add `getScrollResponder` to all ScrollView-like components, including ListView. This allows multiple higher-order scroll views to be composed while allowing the owner of the top-level scroll view to call `scrollableView.getScrollResponder().scrollTo(...)` regardless of whether `scrollableView` is a ScrollView, ListView, InvertedScrollView, etc.
Closesreact#766
Github Author: James Ide <ide@jameside.com>
Test Plan: Imported from GitHub, without a `Test Plan:` line.
@ideide closed this in 0119998Jun 12, 2015
@ide
ide deleted the listview-api-2 branch June 13, 2015 01:44
Fanghao pushed a commit to discord/react-native-archived that referenced this pull request Jul 23, 2015
…tion
Summary:
This is a proposal to add `getScrollResponder` to all ScrollView-like components, including ListView. This allows multiple higher-order scroll views to be composed while allowing the owner of the top-level scroll view to call `scrollableView.getScrollResponder().scrollTo(...)` regardless of whether `scrollableView` is a ScrollView, ListView, InvertedScrollView, etc.
Closesreact#766
Github Author: James Ide <ide@jameside.com>
Test Plan: Imported from GitHub, without a `Test Plan:` line.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ide@nicklockwood@vjeux@sahrens@facebook-github-bot@frantic