Support RefreshControl in RecyclerViewBackedScrollView in Android

[tegon]

In Android, RecyclerViewBackedScrollView wasn't using refreshControl prop.
If a ListView were created with RecyclerViewBackedScrollView as its renderScrollComponent, then the refreshControl wouldn't work.
example:

        <ListView
          dataSource={this.props.dataSource}
          renderRow={this._renderRow.bind(this)}
          refreshControl={
            <RefreshControl 
              refreshing={this.props.isRefreshing} 
              onRefresh={this._onRefresh.bind(this)} 
            />
          }
          renderScrollComponent={props => <RecyclerViewBackedScrollView {...props} />}/>;

This works in iOS, since the RecyclerViewBackedScrollView just returns an ScrollView.

This pull request uses the refreshControl to decide whether it should wrap the NativeAndroidRecyclerView with an
AndroidSwipeRefreshLayout or not.

This fixes the issue #7134.

[ghost]

By analyzing the blame information on this pull request, we identified @kmagiera and @jutaz to be potential reviewers.

[satya164]

cc @janicduplessis

[janicduplessis]

Can you add a style with flex: 1 to NativeAndroidRecyclerView to make sure it fills the AndroidSwipeRefreshLayout.

[tegon]

This style is already in props.

console.log('props.style', props.style[0])
=> props.style Object {flex: 1}

[janicduplessis]

I'm mostly concerned about passing props.style to both AndroidSwipeRefreshLayout and NativeAndroidRecyclerView. Styles like margin could cause issues because they will get applied twice. Adding style after ...props will prevent that.

[tegon]

Ok, got it. Just added the style in the last commit.

[ide]

Can you remove props.style above in theprops definition and instead pass in this.props.style (and add flex: 1 if it's necessary).

And set style in the props definition above to just {flex: 1}.

Also rename props to recyclerProps.

[tegon]

Sorry, i'm a litte bit confused... it should became something like this?

    const refreshControl = this.props.refreshControl;
     if (refreshControl) {
       // Wrap the NativeAndroidRecyclerView with a AndroidSwipeRefreshLayout.
       return React.cloneElement(
         refreshControl,
         {style: {flex: 1}},
         <NativeAndroidRecyclerView {...props}>
           {wrappedChildren}
         </NativeAndroidRecyclerView>
       );
     }

[janicduplessis]

I think the clearer way to do this is not include style in the props declaration (line 95). This way we can be explicit about what style we want for each case.

Should look like

   const recyclerProps = { ... };

   ...

    const refreshControl = this.props.refreshControl;
     if (refreshControl) {
       // Wrap the NativeAndroidRecyclerView with a AndroidSwipeRefreshLayout.
       return React.cloneElement(
         refreshControl,
         {style: [styles.base, this.props.style]},
         <NativeAndroidRecyclerView {...recyclerProps} style={styles.base}>
           {wrappedChildren}
         </NativeAndroidRecyclerView>
       );
     }

styles.base being {flex: 1}

We'll also have to change the case where there is no refreshControl to style={[styles.base, this.props.style]} and {...recyclerProps}

[ide]

Yeah, one way to think of it is: how would this code be most clearly written from scratch?

[tegon]

Ok, I've understood now, it is more clear indeed. Just pushed the changes.

[ide]

Thanks, this looks good!

@facebook-github-bot shipit

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[waqas19921]

is that issue fixed?
refreshcontrol is still not working if ListViewwere created with RecyclerViewBackedScrollViewas its renderScrollComponent
i am using react-native 0.30

[janicduplessis]

This is in 0.31.0-rc