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.

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

Fix spinner is not shown on beginRefreshingProgrammatically on IOS #27236

Conversation

IgnorancePulls
Copy link

Summary

It closes #24855
In the endRefreshProgrammatically of RCTRefreshControl.m there is calculation for content offset when spinner is shown CGPoint offset = {scrollView.contentOffset.x, scrollView.contentOffset.y - self.frame.size.height};
However self.frame.size.height is always 0 and therefore spinner is not visible
This change should fix that

Changelog

[iOS] [Fixed] - Fix spinner visibility on beginRefreshingProgrammatically

Test Plan

IOS tests passed
Check whether this issue is reproduced or not for the repro which is described inside the issue.
#24855

@facebook-github-bot
Copy link
Contributor

Hi IgnorancePulls! 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 at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@react-native-bot react-native-bot added Platform: iOS iOS applications. Bug labels Nov 15, 2019
@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 Nov 15, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@marlonjohnynion
Copy link

Any update regarding this PR?

@hramos
Copy link
Contributor

hramos commented Nov 25, 2019

@ninjaengineers can you fix the conflict in RCTRefreshControl.m? It would be useful to also have some screenshot or video that demonstrates the issue as well as the resulting fix.

@marlonjohnynion
Copy link

@ninjaengineers can you fix the conflict in RCTRefreshControl.m? It would be useful to also have some screenshot or video that demonstrates the issue as well as the resulting fix.

@hramos unfortunately, I only do javascript; however, can we please ask @IgnorancePulls to review his PR and resolve the conflicts?

Copy link

@ghsdh3409 ghsdh3409 left a comment

Choose a reason for hiding this comment

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

I checked that this PR fixes the issue (#25898).

cc. @hramos @IgnorancePulls

@IgnorancePulls
Copy link
Author

@ghsdh3409 thank you!

@hramos
Copy link
Contributor

hramos commented Nov 26, 2019

@IgnorancePulls can you fix the conflict in React/Views/RCTRefreshControl.m?

@ghsdh3409
Copy link

@IgnorancePulls Could you resolve merge conflict?

@IgnorancePulls
Copy link
Author

@IgnorancePulls Could you resolve merge conflict?

Unfortunately i would not be able to do it any time soon, could you take it over from here?

@ghsdh3409
Copy link

@IgnorancePulls Okay. But, I have no authorization for this PR. Can I open new PR for this change?

@nnabinh
Copy link
Contributor

nnabinh commented Dec 2, 2019

Waiting to get this fix soon. Thanks!

@nnabinh
Copy link
Contributor

nnabinh commented Dec 3, 2019

@IgnorancePulls @hramos @ghsdh3409
Hey guys, since I really want to get this fix merged soon, I created another PR here due to authorization.
Please let me know if there is any issue with it.

@ghsdh3409
Copy link

@nnabinh Good! Thanks!

facebook-github-bot pushed a commit that referenced this pull request Dec 6, 2019
…27397)

Summary:
It closes #24855
In the endRefreshProgrammatically of RCTRefreshControl.m there is calculation for content offset when spinner is shown CGPoint offset = {scrollView.contentOffset.x, scrollView.contentOffset.y - self.frame.size.height};
However self.frame.size.height is always 0 and therefore spinner is not visible
This change should fix that

Since the owner of the following PR is quite busy and won't be able to resolve the merge conflict anytime soon, I created this PR here to get the fix merged soon.
Ref: #27236
Thanks to [IgnorancePulls](https://github.com/IgnorancePulls)

## Changelog

[iOS] [Fixed] - Fix spinner visibility on beginRefreshingProgrammatically
Pull Request resolved: #27397

Test Plan:
IOS tests passed
Check whether this issue is reproduced or not for the repro which is described inside the issue.
#24855

Reviewed By: sammy-SC

Differential Revision: D18801307

Pulled By: hramos

fbshipit-source-id: d12af236778441a136dbe6b03dfd3495a465ae0f
@ghsdh3409
Copy link

This PR was reopened and merged in #27397.
We can close this PR.

@shergin shergin closed this Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RefreshControl does not work correctly on state change
8 participants