Cross platform PullToRefreshView component #4915
Conversation
|
By analyzing the blame information on this pull request, we identified @nicklockwood, @gabelevi and @andreicoman11 to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
I agree with the idea, and it's great that you're working on this. Probably worth reading this discussion though: #4205 I think we were leaning towards having the RefreshControl be a child or property of the ScrollView, rather than a wrapper. This matches the underlying implementation on iOS, and although Android uses a wrapper under the hood, conceptually it makes more sense that a control that appears inside a scrollview should be a child of it rather than its parent. |
|
I agree with @nicklockwood. I think it makes more sense to have it be a property of the ScrollView. If we had other components that had this behavior it would make more sense to have a wrapper view for something, but this is pretty much just ScrollView. Incidentally, do HorizontalScrollViews support this in iOS? |
|
@dmmiller iOS scrollViews are bidirectional - there's no separate horizontal scrollview control. But I'm pretty sure PTR only works when you pull down. I don't know what happens if you add it to a scrollview whose contentSize is wider than the view. |
|
@nicklockwood I agree that a child or a prop would be a better api. I did it that way initially since it was easy to make the ios api be like the android one. I'll give it a try to what you suggested. Few questions to help me get started: Any preference for a child vs a prop on scrollview? Personally I like not having to add the extra prop on scrollview. Should i make 2 different components for ios and android (RefreshControlIOS, RefreshControlAndroid) or only one with platform specific props to customize the look? How would you go roughly about implementing this? I'm pretty new to react native development so I want to be sure to implement it the best way possible. I think I got a good idea on how to do it on ios but for android I'm not sure. Is the an other component that wraps itself with another view if it contains a specific prop? |
I'd prefer a child. I think @ide preferred a prop. For the most part it makes little difference to the underlying implementation though (the RefreshControl component will in fact have to be a child, even if we don't expose that in the public API).
The latter, definitely.
We have a few examples of this sort of thing on iOS already (e.g how RCTText or RCTMap handles subviews), but basically in insertReactSubview on RCTScrollView, you'd detect if the view was a subclass of UIRefreshControl, and if so you'd attach it as the refreshControl for the view, otherwise add it to the contentView as normal.
I think on Android the ScrollView.js would probably detect if you'd added the RefreshControl child and then return the PullToRefreshView with a ScrollView inside it. A bit like how in Image.android.js, it returns a view with an RCTImage inside it if the image has children. (I'm not an Android expert though, so perhaps defer to @dmmiller on that one). |
|
Sounds good, I started the iOS implementation and created the native RefreshControl component I'll let you know if any issue arises. For Android I had the same idea, it should be pretty simple to implement it in ScrollView.js. |
|
@janicduplessis updated the pull request. |
1 similar comment
|
@janicduplessis updated the pull request. |
janicduplessis
force-pushed
the
pull-to-refresh-2
branch
from
caaeb2b to
abf9c31
Compare
|
I implemented it in iOS using a child component and added support for tintColor and title. Title is an NSAttributedString is there any better way to support this? Right now I'm only passing a NSString so there is no way to change the style of the title. I'll work on the Android part later today. |
|
@janicduplessis the only way to do attributed text would be to permit a |
|
@janicduplessis updated the pull request. |
|
@nicklockwood Yes this should probably be in another pull request since it is less important and this one is getting bigger :). I have done the Android implementation, I'm not 100% satisfied about it but it works well. Not sure if I should split it in 2 components for Android, one that is the actual native pull to refresh view rendered only by the scrollview and the other one that acts only as a configuration object that doesn't render anything and is put as a child of the scrollview. Right now I use the same component for both and only return null in render if there is no child component. Using 2 components would also allow to remove the ...View.propTypes from the RefreshComponent which doesn't really makes sense since it's not really a view in iOS. |
|
@janicduplessis updated the pull request. |
2 similar comments
|
@janicduplessis updated the pull request. |
|
@janicduplessis updated the pull request. |
janicduplessis
force-pushed
the
pull-to-refresh-2
branch
from
a65a764 to
eb8b61a
Compare
|
@janicduplessis updated the pull request. |
janicduplessis
force-pushed
the
pull-to-refresh-2
branch
from
eb8b61a to
d6c07a5
Compare
|
@janicduplessis updated the pull request. |
|
Ok, I cleaned everything up if anyone is interested in reviewing this. We should also settle on prop on ScrollView vs child component. One advantage of the prop for the implementation is it would avoid having to loop through the children of the ScrollView to find the RefreshControl. cc @nicklockwood, @ide, @dmmiller |
When put inside a ScrollView it will add a UIRefreshControl on iOS and a SwipeRefreshLayout on Android. Adds support for tintColor and title on iOS and all the props currently supported on PullToRefreshViewAndroid on Android. This will allow to deprecate/remove the onRefreshStart prop on ScrollView and PullToRefreshViewAndroid in a future PR.
janicduplessis
force-pushed
the
pull-to-refresh-2
branch
from
d6c07a5 to
c632f4d
Compare
|
@janicduplessis updated the pull request. |
| * The color of the refresh indicator. | ||
| * @platform ios | ||
| */ | ||
| tintColor: React.PropTypes.string, |
There was a problem hiding this comment.
This should be ColorPropType (sorry, it probably changed since you wrote this).
|
@nicklockwood I think I like it better as a component just for the fact that it will allow to separate the doc and not clutter the ScrollView API too much. |
|
@janicduplessis There's no reason why it couldn't be in a separate file with its own docs just because it's a regular object, is there? |
|
@nicklockwood I don't think so, it could be a module that exports a RefreshControlPropType with the same shape as the current RefreshControl component. Not sure where is the best place to put the SIZE constants that are used on Android though. |
…pes, fix reactSubviews
|
@janicduplessis updated the pull request. |
|
Thanks for reviewing this, I made the adjustments and changed it from a child component to a prop on ScrollView. I also tested adding/removing the RefreshControl on iOS to make sure it was working. I left RefreshControl as a component for now until we figure out what we want to do. |
|
+1. We need unified API for pull to refresh control on both iOS and Android. Happy to see this. |
|
Any reason why the |
|
+1 to making |
|
@ide Cool, I'll change that when I ship it. @janicduplessis, if you'd strongly prefer a function, feel free to add a |
|
@facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/161752757518060/int_phab to review. |
ghost
closed this
in
|
No, I don't know why I made it a function it is not really needed. |
|
Do we want to add a deprecation warning to the old APIs that points to this component? I could do that in another PR. |
|
I'm not sure which APIs you're referring to. Do you mean the onRefreshStart callback? |
|
onRefreshStart on ScrollView and PullToRefreshViewAndroid. |
|
I've added a deprecation warning to onRefreshStart already. Not done anything with PullToRefreshAndroid yet though. |
|
@janicduplessis @nicklockwood When would these be released? As a part of v0.18 or something? I'm on 0.17 right now and would love to use this -- maybe you could make this into its own |
|
@niftylettuce It will part of 0.18.0, right now it is available as a release candidate on npm (0.18.0-rc). The doc isn't showing on the website yet but you can check https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/RefreshControlExample.js for an example of how to use it. |
|
@janicduplessis Any chances that RefreshControl will be implemented also for ListView? My concern is performance. ListView should be me suitable for large datasets |
|
@mauron85 It is, ListView supports all ScrollView props :) |
|
Ach. cool then. :-) Thanks |
|
Ok. One more question. Is it possible to pull to refresh empty ListView? I've experience with some modules, where it was not possible. EDIT: Possible with this module https://github.com/moschan/react-native-pull-to-refresh, as it wraps ListView. Seems to NOT be possible with RefreshControl as empty ListView/ScrollView has 0 height. Or I'am wrong? |
|
I'm pretty sure it is but you'd have to test it. |
|
Just did. Didn't :-( |
|
I can check tonight, if it's not possible it's probably a bug. |
|
Sorry. Feeling stupid. But I was wrong. It works just fine. |
|
All good :) |
Both iOS and Android currently support some sort of native pull to refresh control but the API was very different. I tried implementing a component based on PullToRefreshViewAndroid but that works on both platforms.
I liked the idea of wrapping the ListView or ScrollView with the PullToRefreshView component and allow styling the refresh view with platform specific props if needed. I also like the fact that 'refreshing' is a controlled prop so there is no need to keep a ref to the component or to the stopRefreshing function.
It is a pretty rough start so I'm looking for feedback and ideas to improve on the API before cleaning up everything.
On iOS we could probably deprecate the onRefreshStart property of the ScrollView and implement the native stuff in a PullToRefreshViewManager. We could then add props to customize the look of the UIRefreshControl (tintColor). We could also deprecate the Android only component and remove it later.