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

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

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Apr 9, 2015

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-bot facebook-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
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
Copy link
Contributor Author

ide commented Apr 9, 2015

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:

class InfiniteScrollView extends React.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);
  }
};

@ide ide force-pushed the listview-api-2 branch from 0b869f5 to 7d09f4c Compare April 10, 2015 08:27
@nicklockwood
Copy link
Contributor

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

@vjeux
Copy link
Contributor

vjeux commented Apr 11, 2015

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
Copy link
Contributor Author

ide commented Apr 11, 2015

@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
Contributor

vjeux commented Apr 11, 2015

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
Copy link
Contributor Author

ide commented Apr 13, 2015

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

<ScrollViewA
  renderScrollView={() =>
    <ScrollViewB renderScrollView={() => <ScrollViewC/ >} />
  }
/>

@ide ide force-pushed the listview-api-2 branch 3 times, most recently from 6286480 to bf524ee Compare April 15, 2015 20:59
@ide ide force-pushed the listview-api-2 branch 2 times, most recently from bc49683 to a02056f Compare April 28, 2015 01:24
@ide ide force-pushed the listview-api-2 branch from a02056f to 9dfaaca Compare April 30, 2015 05:36
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 ide force-pushed the listview-api-2 branch from 9dfaaca to fd2ba8b Compare May 8, 2015 11:34
@brentvatne brentvatne changed the title [RFC][ScrollView] Add getScrollResponder to ScrollView for composition [Idea: ScrollView] Add getScrollResponder to ScrollView for composition Jun 1, 2015
@sahrens sahrens assigned frantic and sahrens and unassigned frantic Jun 11, 2015
@sahrens
Copy link
Contributor

sahrens commented Jun 11, 2015

Oops, wrong PR.

@sahrens
Copy link
Contributor

sahrens commented Jun 11, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
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.
Closes facebook#766
Github Author: James Ide <[email protected]>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
@ide ide closed this in 0119998 Jun 12, 2015
@ide ide deleted the listview-api-2 branch June 13, 2015 01:44
Fanghao pushed a commit to discord/react-native 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.
Closes facebook#766
Github Author: James Ide <[email protected]>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020
Ran into this problem while following this guide verbatim.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This 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