Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fix] RefreshControl Position Bug #9249

Closed
wants to merge 11 commits into from
Closed

[Fix] RefreshControl Position Bug #9249

wants to merge 11 commits into from

Conversation

GantMan
Copy link
Contributor

@GantMan GantMan commented Aug 5, 2016

Refresh bug #7976
This bug has persisted for 3 versions of React Native. Currently everyone is having to do a silly fix, documented in the bug. I spent an hour trying to find the source of the bug. Failing to find the issue quickly, I've just decided to make this simple bugfix.

According to @janicduplessis #7976 we will likely be re-writing this control in iOS 10.

@ghost
Copy link

ghost commented Aug 5, 2016

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

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 5, 2016
eslint-bot
eslint-bot reviewed Aug 5, 2016
View reviewed changes
Libraries/Components/RefreshControl/RefreshControl.js
@@ -149,9 +149,11 @@ const RefreshControl = React.createClass({
},

render() {
var adjustedProps = {...this.props}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semi: Missing semicolon.

@GantMan
Copy link
Contributor Author

GantMan commented Aug 5, 2016

Will update and fix. I forgot you guys love the semicolon!

@ghost
Copy link

ghost commented Aug 7, 2016

@GantMan updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Aug 7, 2016

@GantMan updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 7, 2016
@ghost
Copy link

ghost commented Aug 7, 2016

@GantMan updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 7, 2016
@ghost
Copy link

ghost commented Aug 8, 2016

@GantMan updated the pull request.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@GantMan updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 8, 2016
@janicduplessis
Copy link
Contributor

Have you tried doing this fix in objc since it is only needed for iOS? Maybe try if (backgroundColor == nil) backgroundColor = [UIColor clearColor]; somewhere in RCTRefreshControl.

If that doesn't work let's add a Platform.OS check in JS to make sure it only runs on iOS.

@ghost ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 17, 2016
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 17, 2016
@GantMan
Copy link
Contributor Author

GantMan commented Aug 20, 2016

sounds good, gonna check on that now.

@ghost
Copy link

ghost commented Aug 20, 2016

@GantMan updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2016
@ghost
Copy link

ghost commented Aug 20, 2016

@GantMan updated the pull request - view changes

@GantMan
Copy link
Contributor Author

GantMan commented Aug 20, 2016

@janicduplessis - you were right. The iOS fix works great. This also helps keep the clutter out of JavaScript. Tested this in a local app, it worked perfect.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2016
@ghost
Copy link

ghost commented Aug 22, 2016

@GantMan updated the pull request - view changes

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 22, 2016
@ghost
Copy link

ghost commented Aug 31, 2016

@GantMan updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 31, 2016
janicduplessis
janicduplessis reviewed Aug 31, 2016
View reviewed changes
React/Views/RCTRefreshControl.m
@@ -33,6 +33,9 @@ - (void)layoutSubviews
{
[super layoutSubviews];

// Quick fix for iOS backgroundColor bug #7976 - To be removed on update
if (self.backgroundColor == nil) self.backgroundColor = [UIColor clearColor];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: put if content on new line with brackets.

@janicduplessis
Copy link
Contributor

Just a few comments inline then this is good to go :)

@ghost
Copy link

ghost commented Sep 4, 2016

@GantMan updated the pull request - view changes

@GantMan
Copy link
Contributor Author

GantMan commented Sep 4, 2016

Hey @janicduplessis feedback incorporated. Sorry for the delay, I didn't see the comments in my notifications 👍 :shipit:

@ghost
Copy link

ghost commented Sep 4, 2016

@GantMan updated the pull request - view changes

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 4, 2016
@janicduplessis
Copy link
Contributor

@GantMan Thanks!

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Sep 4, 2016
@ghost
Copy link

ghost commented Sep 4, 2016

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

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 4, 2016
@ghost ghost closed this in dcdf16a Sep 4, 2016
@GantMan GantMan deleted the fix/refreshcontrol branch September 4, 2016 21:36
@kamui545 kamui545 mentioned this pull request Feb 8, 2021
Closed
@swrobel swrobel mentioned this pull request Mar 5, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved. p: Infinite Red Partner: Infinite Red Partner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GantMan @facebook-github-bot @janicduplessis @eslint-bot