UIRefreshControl added to scroll view

[EwanThomas]

What:

adds onRefreshStart property to ScrollView.js for displaying and activating pull to refresh.

Why:

Javascript implementations seemed a little flakey and inconsistent. As you can see in the issues below:

#2356
#745

So this is an attempt a completely native implementation.

What do you think?

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @sahrens, @nicklockwood and @tadeuzagallo to be potential reviewers.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[facebook-github-bot]

@EwanThomas updated the pull request.

[nicklockwood]

This should probably check if onRefreshStart is nil and remove the refreshControl if it is.

Also, if onRefreshStart is not nil, and _scrollView.refreshControl is already not nil, there's no need to create and assign a new UIRefreshControl.

[EwanThomas]

updated

[nicklockwood]

This seems pretty reasonable to me. I wonder if refreshing should be a controlled property instead of a method, but I'm not sure.

@vjeux, do you have an opinion about the JS API?

[facebook-github-bot]

@EwanThomas updated the pull request.

[facebook-github-bot]

@EwanThomas updated the pull request.

[vjeux]

@ide and @sahrens are better reviewers as they wrote such feature in the past

[ide]

I am fairly hesitant about adding an un-extensible API to as fundamental of a building block as ScrollView. There's no way to customize the PtR behavior such as how the refresh indicator looks or how it responds to the gesture. I would prefer this to go in a new component like RefreshableScrollViewIOS and ultimately to be able to use JS to implement PtR -- as we've seen repeatedly it's so much nicer to be using pure JS components once we get them working well.

[nicklockwood]

I agree that extensible JS-based features are the ideal, but when complex platform features already exist that can be trivially exposed, I think it makes sense to do so.

This isn't something that can be easily implemented via a plugin, it doesn't add much native code, and it doesn't make it harder to implement a JS-based alternative. For those reasons, I think it makes sense to add it to the core.

[pietropizzi]

@ide

[...] ultimately to be able to use JS to implement PtR

I agree that this is where we should be heading and that was also the initial way we wanted to implement PtR. Only after spending lots of time with many different components, trying to implement it ourselves did we give up. If you have any new insights into how to do this right, we're all 👂s

To @nicklockwood's point though, this is a rather small change set and it would allow us to use a battle tested simple to use component today.

I'd be totally happy to deprecate this once a solid path in JS is there and simple to use.

[ide]

Yeah it's a pain in the butt to get pure JS PtR working. I have an open-sourced solution that's 90% of the way there but I don't love it and it kind of undermines the level of quality that iOS apps provide. Two questions - is there a way to add this to core without adding it to ScrollView specifically? and what does the Android API look like?

[ide]

use arrow functions instead of bind

[nicklockwood]

An alternative implementation might be to add a <PullToRefreshIOS> control that can be added to <ScrollView> and implements the associated events on itself. I think I slightly prefer that as an API, as it avoids polluting the ScrollView JS API with another platform-specific property.

I don't know enough about Android to be able to say if an equivalent would be possible there, but if Android doesn't have a native PTR control then that doesn't prohibit us from exposing one on iOS - we want x-platform consistency, but we aren't going for lowest-common-denominator.

[ide]

Android has a native PtR control but the API is a little different. It's called SwipeRefreshLayout ("Layouts" in Android are subclasses of ViewGroup, which can have subviews while Views cannot have subviews..... yeah.) Anyway, the idea is you might write something like this:

<SwipeRefreshLayout>
  <ScrollView />
</SwipeRefreshLayout>

cc @dmmiller for how FB's Android refresh API actually looks

[nicklockwood]

Hmm, well it's kind of ass-backwards from my perspective, but there's no technical reason why we couldn't implement PTR as a container around ScrollView on iOS as wel, as opposed to a property or child component. The container would just set the refreshControl property on its child.

[ide]

Yeah. It's a little strange when you think about it from the perspective of the view hierarchy -- even on Android the SwipeRefreshLayout needs to know about the scroll offset of its child ScrollView. That said I do like how two components are composed, rather than adding more functionality to one component.

[pietropizzi]

In this case the composition will not lead us anywhere though and will just give us the illusion of decoupling since in iOS you can only add the UIRefreshControl as a subview of a UIScrollView (or a UITableView, but that just uses a UIScrollView).

So adding one more layer to the view hierarchy won't change this hard dependency, we would still have to pass all this down to the actual UIScrollView inside of RCTScrollView inside of RCTCustomScrollView.

Or am I missing something?

[nicklockwood]

@pietropizzi it could be modified to work with other views in future, such as a hypothetical RCTTableView. It wouldn't necessarily be decoupled on the native side (although it could be, if we coded to it work with any component that conforms to a "RCTRefreshableView" protocol), but decoupling on the JS side is still potentially valuable as it means you could use it with any custom component that renders a ScrollView (this would also be true it if was a property of ScrollView of course, but it sounds like that might be harder to implement on Android).

[facebook-github-bot]

@EwanThomas updated the pull request.

[dmmiller]

@ide I don't actually know too many details about how Android SwipeRefreshLayout works. Though reading the conversation it seems like iOS and Android are just very different in how it is done, at least at the view level. We should probably figure out the right way to expose it in js and then make it work on each platform as best we can.

[pietropizzi]

@nicklockwood Ok, so if we should rewrite this in another way I just want to make sure I understand how, because I currently don't.

How would you add a potential <PullToRefreshIOS> view to another view then.

Are you thinking like this:

<ScrollView>
  <PullToRefreshIOS onRefresh={() => { /* do something */ }} />
  <MyOtherStuff />
</ScrollView>

in which case how would this for example work with a list listview? Because that replaces the stuff inside the <ScrollView>

or more like:

<ScrollView
  renderPullToRefresh={() => <PullToRefreshIOS onRefresh={() => { /* do something */ }} /> }
>
  <MyOtherStuff />
</ScrollView>

If I have a <PullToRefreshIOS> component what would prevent someone from doing this

<View>
  <PullToRefreshIOS>
</View>

This would not work in the iOS side, because we need to make sure that we only add it to UIScrollView.

Any steps / blueprints to how to do this are appreciated.

[EwanThomas]

Are there any existing examples that we could dissect of the below proposed pattern? So we can get a clearer understanding.

<SwipeRefreshLayout>
  <ScrollView />
</SwipeRefreshLayout>

[nicklockwood]

@EwanThomas you mean examples of components that can only be nested inside a specific parent, or only accept a specific child?

<TabBarIOS>
  <TabBarItemIOS/>
</TabBarIOS>

[pietropizzi]

@nicklockwood

We are mostly trying to understand how to implement this without adding a new prop to the JS ScrollView:

When using our not yet created component:

<RefreshableContainer onRefreshStart={(endRefreshing) => {
  // After some time call endrefereshing
  setTimeout(endRefreshing, 3000);
}}>
  <ScrollView />
</RefreshableContainer>

it's render method:

render() {
  return (
    React.cloneElement(React.Children.only(this.props.children), {
      onRefreshStart: this.props.onRefreshStart
    })
  );
}

We still end up adding onRefreshStart to the ScrollView. How else could you do it?

Would you have a Native UI Component for RefreshableContainer that then talks to the Native ScrollView Component instead?

[nicklockwood]

@pietropizzi I think you're assuming that the RefreshableContainer component would exist only on the JS side. But I'm assuming it would be backed by a native view/viewmanager, in which case the native RefreshableContainer would be the receiver for the UIRefreshControl's UIControlEventValueChanged event, and would therefore be able to send the onRefreshStart event directly, without needing the ScrollView to do it.

[nicklockwood]

OK, I spoke with @dmmiller and we agreed that having this as a property of ScrollView is more intuitive than a wrapper component, and that it can be implemented with the same API on Android by doing the component wrapping in the JS file if the property is set.

We've no immediate requirement for this internally, so if nobody has any objections, I'm going to accept this PR as-is and file a task to port it to Android at a later date.

If someone comes up with a better API or a flawless JS-based version in the meantime, we'll just deprecate the property.

[ide]

👍🏻

[nicklockwood]

@facebook-github-bot shipit

[facebook-github-bot]

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

[pietropizzi]

@nicklockwood Great news! :)

Did this get closed instead of merged because of the missing CLA thing? We did sign the CLA, but somehow it did not work.

[ide]

The bot closed this PR because it was merged :)

[pietropizzi]

@ide Was just confused because some PRs are marked as merged and some as closed. But sweet then :)

[janmonschke]

🎉

[ashleydw]

@EwanThomas there doesn't seem to be any way of accessing the UIRefreshControl via the ScrollView, and therefore no way to style it?

We should be able to set the properties defined in the documentation: https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIRefreshControl_class/

[nicklockwood]

@ashleydw this is a deliberately simple API, designed to make it easier to port to Android, which has a wildly implementation than iOS.

I think the main properties you might want to set on the iOS refresh control are the title and tintColor, correct?

If so, I see two options: Either we add two additional iOS-specific properties to ScrollView, which I'm not keen on, since it bloats the ScrollView API. Or, we could add a RefreshControlIOS that has these properties, and can be added as a child of the ScrollView. I think I'd prefer that, since it keeps the platform-specific stuff out of the shared API.

@ide, @dmmiller - any thoughts?

[ide]

Without considering consistency with Android, I think RefreshControlIOS is a good way to approach the API:

<ScrollView
  renderRefreshControl={() => <RefreshControlIOS style={styles.spinner} />}>
  {children}
</ScrollView>

This way you could maybe even subclass UIRefreshControl, bridge the subclass to JS, and inject it into the ScrollView from React.

The Android API is considerably different, though (https://github.com/facebook/react-native/blob/master/Libraries/PullToRefresh/PullToRefreshViewAndroid.android.js). Usage looks like this:

      <PullToRefreshLayoutAndroid
        style={styles.layout}
        refreshing={this.state.isRefreshing}
        onRefresh={this._onRefresh}
        colors={['#ff0000', '#00ff00', '#0000ff']}
        progressBackgroundColor={'#ffff00'}>
        <ScrollView>
          {children}
        </ScrollView>
      </PullToRefreshLayoutAndroid>

I'm somewhat confident we could translate one API style to the other. Ex: on Android if ScrollView were to support a renderRefreshControl prop, we'd extract properties from the refresh control React component (which doesn't map to any native component) and use those properties to configure a PullToRefreshLayoutAndroid wrapper. I haven't used either of these APIs in practice to have an informed sense of which is preferable though.

[nicklockwood]

@ide Is having a renderRefreshControl property preferable to just doing this?

<ScrollView>
  <RefreshControlIOS style={styles.spinner} />
  {other_children}
</ScrollView>

In this case, we probably can't defer rendering the control until the point of activation, since it must be set on the scrollview in advance in order to make it respond to the refresh gesture in the first place, so adding the extra indirection seems unnecessary.

But perhaps it's better for some other reason I'm not familiar with?

[jbrodriguez]

What if you want to refresh a ListView rather than a ScrollView ?

I've just tried the new PullToRefreshViewAndroid with a ListView and it doesn't scroll its children. I'll try it with the example (refreshing a ScrollView) and see if it's something I'm doing wrong.

But I think it'd be more functional a composition approach, a la ...

<SwipeRefreshLayout> (or some other name)
  <ScrollView /> (or <ListView />)
</SwipeRefreshLayout>

[nicklockwood]

ListView has the same native implementation as ScrollView (on iOS, at least), so it should just work the same.

[ide]

@jbrodriguez ListView simply wraps a ScrollView. So you could probably inject in a ScrollView that's been wrapped with PullToRefreshViewAndroid.

@nicklockwood I think that would be fine if the refresh control and the other children are truly siblings of each other in the native hierarchy. If memory serves, the children get added to a separate content view, though. What I'd like to avoid is code that checks if the first child is a refresh control and does something differently -- what we want are named children but React doesn't support that.

Conceptually I think we want to write:

<ScrollView
  renderRefreshControl={() => <RefreshControlIOS />}
  children={other}
/>

so that the role of each component is clear. (I use renderRefreshControl instead of refreshControl so that ScrollView can pass in props to the renderRefreshControl if that's one day desirable.) This approach also works well with static analysis. Using JSX's special syntax for children we get:

<ScrollView renderRefreshControl={() => <RefreshControlIOS />}>
  {children}
</ScrollView>

[nicklockwood]

@ide, they would be siblings as far as React is concerned, but we'd treat them differently in the insertReactSubview method of RCTScrollView. The treatment would be based on the view class rather than the position in the subview array, so it would be pretty robust (we already use this approach in a bunch of places, e.g text nodes inside textinput, or image nodes inside text).

[ide]

I think I prefer the separate prop for its straightforwardness (though thinking about it some more refreshControl is probably better than renderRefreshControl in this case since we are just trying to add a special child component). In the case of text nodes or inline, the components' position is important but in this case we want to specify the one and only refresh control.

[alvinwoon]

Thanks for adding UIRefreshControl to 0.16! Is changing the tintColor being exposed to React Native?

[nicklockwood]

@alvinwoon that's what we've been discussing in the thread above.

[alvinwoon]

My bad! It was a trail of GitHub threads that led me here. Thanks again! This is so much smoother than all the 3rd party modules out there.