[ListView] Allow different types of ScrollView to be composed

[ide]

This enables code like:

<ListViewrenderScrollView={()=><CustomScrollView/>}/>

where CustomScrollView might be inverted or support pull-to-refresh, etc.

[ide]

cc @vjeux, this implements the composition prop discussed in #766

[ide]

It could be better to name the prop renderScrollComponent or renderScrollableView since it's OK to return components that aren't exactly <ScrollView>. Is renderScrollView confusing?

[vjeux]

why do you need to do that?

[ide]

I want to merge the ListView's props (e.g. scrollEventThrottle) with the underlying ScrollView's props if renderScrollView returned something like <ScrollView pageSize={10} />, but the implementation in this diff is not flexible enough. I'll update it to your renderScrollView(props) idea after I try composing some ScrollViews together.

[ide]

catping

[frantic]

I tried this some time ago, but it turned out to be a disaster - RefreshableScrollView scrollview didn't play nicely with ListView because ListView depends on stickyHeaderIndices prop of ScrollView and assumes there are no other views besides those managed by ListView. Unfortunately RefreshableScrollView does add extra views and shifts those indices, resulting in incorrect ListView rendering...

@ide have you tried

<ListViewrenderScrollView={()=><RefreshableScrollView/>}/>

with sections that have sticky headers?

[ide]

@frantic: my RefreshableScrollView (not yet open-sourced) doesn't modify the header indices or children so composing it with a ListView works. You may actually want to look at using it when it is open-source since it handles more edge cases and features than I believe the FB implementation does. Along with #766 this composition API lets me create reversed, refreshable, infinite-scroll list views for example.

[frantic]

@ide mind sharing it? I guess we could merge it into the main repo if you want.

[ide]

Yep, my plan is to share it in the near future once it is easy to use out of the box, since I am still working on the default spinner component. I will probably publish it to npm since it is pure JS code; it will be as simple as npm install react-native-refreshable-scroll-view and no need to add code to core.

[sahrens]

No reason to mark this as required since it has a default implementation.

[sahrens]

Also, I agree that "ScrollView" is a little confusing here - renderScrollComponent might be better. Can you update?

[ide]

Will update with a renamed version.

Regarding isRequired, I understand it to mean that the type of this.props.renderScrollView is Function instead of ?Function in Flow-speak. So because we're saying the prop is non-null (which is true because of the default value from getDefaultProps), Flow won't complain about this.props.renderScrollView() needing a null check.

[sahrens]

Oh, interesting, does flow even apply to non-es6 class-style propTypes?
On Thu, Jun 11, 2015 at 4:29 PM James Ide notifications@github.com wrote:

In Libraries/CustomComponents/ListView/ListView.js
#785 (comment):

@@ -164,6 +164,13 @@ var ListView = React.createClass({
/
renderSectionHeader: PropTypes.func,
/*

• \* (props) => renderable
  

• *
  

• \* A function that returns the scroll view in which the list rows are
  

• \* rendered. Defaults to returning a ScrollView with the given props.
  

• */
  

• renderScrollView: React.PropTypes.func.isRequired,

Will update with a renamed version.

Regarding isRequired, I understand it to mean that the type of
this.props.renderScrollView is Function instead of ?Function in
Flow-speak. So because we're saying the prop is non-null (which is true
because of the default value from getDefaultProps), Flow won't complain
about this.props.renderScrollView() needing a null check.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/react-native/pull/785/files#r32279234.

[ide]

Yeah, here's a small example:

/** * @flow */'use strict';varReact=require('react-native');var{ PropTypes, Text }=React;varApp=React.createClass({propTypes: {getContent: PropTypes.func,},render(){varcontent=this.props.getContent();return<Text>{content}</Text>;},});

Flow prints:

/Users/ide/test/index.js:15:19,41: call of method getContent
Function cannot be called on possibly undefined value
/Users/ide/test/index.js:11:17,30: undefined

Adding isRequired makes the error go away.

[sahrens]

Oh cool, maybe new flow goodness. But defining a default for the prop and
leaving it not isRequired doesn't make the error go away? cc @glevi
On Thu, Jun 11, 2015 at 8:48 PM James Ide notifications@github.com wrote:

In Libraries/CustomComponents/ListView/ListView.js
#785 (comment):

@@ -164,6 +164,13 @@ var ListView = React.createClass({
/
renderSectionHeader: PropTypes.func,
/*

• \* (props) => renderable
  

• *
  

• \* A function that returns the scroll view in which the list rows are
  

• \* rendered. Defaults to returning a ScrollView with the given props.
  

• */
  

• renderScrollView: React.PropTypes.func.isRequired,

Yeah, here's a small example:

/** * @flow */'use strict';
var React = require('react-native');var { PropTypes, Text } = React;
var App = React.createClass({
propTypes: {
getContent: PropTypes.func,
},

render() {
var content = this.props.getContent();
return {content};
},
});

Flow prints:

/Users/ide/test/index.js:15:19,41: call of method getContent
Function cannot be called on possibly undefined value
/Users/ide/test/index.js:11:17,30: undefined

Adding isRequired makes the error go away.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/react-native/pull/785/files#r32288705.

[ide]

Adding getDefaultProps doesn't make the error go away (Flow 0.11.0):

getDefaultProps(){return{getContent(){return'x';}};},

[bogdantmm92]

Will this ever merge into the main repo?

[sahrens]

Sorry, bit of a holdup but will get it out soon.

-Spencer

On Jun 22, 2015, at 3:06 PM, bogdantmm92 notifications@github.com wrote:

Will this ever merge into the main repo?

—
Reply to this email directly or view it on GitHub.

[brentvatne]

cc @sahrens - this would be nice to get into 0.8.0-rc :)

[ide]

fwiw I'm doing some scroll view work right now and this pattern is working reasonably well in practice e.g. i can make a refreshable, infinitely scrolling list view.

[sahrens]

Sorry, some tests were failing so this got derailed. I'll try to land soon since Animated is mostly done.

On Jul 7, 2015, at 11:35 PM, James Ide notifications@github.com wrote:

fwiw I'm doing some scroll view work right now and this pattern is working reasonably well in practice e.g. i can make a refreshable, infinitely scrolling list view.

—
Reply to this email directly or view it on GitHub.

[paramaggarwal]

Thanks for this @ide! Please do consider removing the isRequired if the Flow issues are fixed!

[ide]

@paramaggarwal Can you send a PR if flow understands getDefaultProps now?

[paramaggarwal]

@ide Oops! I thought you would know - I'm not very familiar with Flow. Sorry, ignore!

[alvaromb]

I'm getting the following warning when wrapping <ListView /> inside a component:

Warning: Failed propType: Required prop `renderScrollComponent` was not specified in `WrappedListViewComponent`. Check the render method of `Component`.

I'm just wrapping the list view to use a custom mixin.

importReact,{ListView}from'react-native'importKeyboardAwareMixinfrom'./KeyboardAwareMixin'constKeyboardAwareListView=React.createClass({propTypes: {
...ListView.propTypes,},mixins: [KeyboardAwareMixin],render: function(){return(<ListViewref='keyboardView'keyboardDismissMode='interactive'contentInset={{bottom: this.state.keyboardSpace}}showsVerticalScrollIndicator={true}{...this.props}/>)},})exportdefaultKeyboardAwareListView

Should I take a different approach or renderScrollComponent should't be required?

[ide]

@alvaromb we could probably make the prop non-required. It was marked as required to tell the Flow type checker that the prop was always defined, but feel free to send a PR to mark it non-required, as long as the type checker passes.