Add progressViewOffset on iOS #11356
Conversation
Fixes #10718. This adds support for the progressViewOffset on the RefreshControl component on iOS.
facebook-github-bot
added
the
CLA Signed
|
@scarlac Hey Seph, is there anyway we could add some tests around these changes? |
|
@ericnakagawa I get where you're going though I'm not sure how to go about it. I could maybe make an assertion so that we're guaranteed that we're repositioning "something" but it wouldn't guarantee it would be the spinner. What's your opinion on that? |
| // UIRefreshControl is managed so avoid changing it directly | ||
| // This method may break in future versions of iOS (subview structure may change) | ||
| // And self.transform won't shift title text | ||
| if(_isInitialRender) { |
There was a problem hiding this comment.
what view are we looking for? can we be more certain that it's the right one.
also I would a better explanation as to what view you're looking for so that whomever has to fix this when it breaks knows what to do :)
There was a problem hiding this comment.
Great suggestion, @mmmulani! I actually forgot about that I could inspect the UI layers.
I'm thinking of this, which seems to work:
NSString *subviewClass = NSStringFromClass([subview class]);
if([subviewClass isEqualToString:@"_UIRefreshControlModernContentView"]) { ... }
I will look into adding a test instead of doing this runtime this week.
There was a problem hiding this comment.
Taking a look at this PR again. I'm concerned about the code here. It's been a while since I've worked on iOS and I am not sure about what current best practices are. @mmmulani do you approve of this method for finding the progress view subview?
There was a problem hiding this comment.
Also, cc @brentvatne since it sounded like you were interested in this PR. Can you take a look?
|
Waiting on tests. |
|
Where can I find some info on writing tests for React Native? |
|
I suggest taking a look at the existing set of tests. They can be found within the Examples/UIExplorer app. |
# Conflicts: # React/Views/RCTRefreshControl.m
|
Should this PR also contain docs for the new prop, as propTypes on the |
|
@scarlac can you update this pr? Otherwise I can fork it and fix the last details? |
|
@mkonicek: I've updated the docs - the prop already exists, so I've just removed the "@platform android" tag. |
|
That works for me. |
|
Looks like it is breaking it, it is not showing refresh indicator while you are scrolling down. |
|
Are there any news about that PR guys ? I'm having the same issue as @arsen here. |
Summary: Previous `contentOffset` can be invalid for a new layout and overscroll the ScrollView, so the diff fixes that. Also documented here: #13566 Reviewed By: mmmulani Differential Revision: D5414442 fbshipit-source-id: 7de1b4a4571108a37d1795e80f165bca5aba5fef
Fixes #10718. This adds support for the progressViewOffset on the
RefreshControl component on iOS.