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

Fixes issue with tipsi-stripe #2838

Merged
merged 1 commit into from
May 24, 2019
Merged

Conversation

ashfurrow
Copy link
Contributor

This PR moves tipsi-stripe to my fork, which includes this commit (copied from this PR that was closed without resolution) which fixes this bug. The bug appears related to our recent React Native upgrade (I ruled out the tipsi-stripe upgrade from 7.4.0 -> 7.5.0).

The fix was to remove a callback for when the keyboard appears; the callback that was being executed was this:

- (void)keyboardWillShow:(NSNotification *)n {
    if (!_jsRequestingFirstResponder && !_isFirstResponder) {
        [_paymentCardTextField resignFirstResponder];
    }
}

Which reads: "if the request to become a first respond (focus the text field) doesn't come from JS and if I am not the first responder (I have the keyboard focus) then make sure that the payment card text field subview loses focus."

I've looked through the git history to figure out the purpose of this code, but it's unclear. My guess is it has to do with supporting credit card inputs in a larger form with other inputs. Our UI has just the credit card input on screen at a time (no other text inputs) so the only way the keyboard shows is if if the _paymentCardTextField has been tapped by the user and is becoming the responder. Because the code only ever makes the payment field resign first responder and is only ever invoked when the payment field becomes the first responder, I am comfortable shipping this. (This also explains the crash we're seeing: stack overflow from infinite show-and-hide-and-show-etc the keyboard.)

Please let me know what I can clarify. I've verified this works as expected on device by going through the complete bid -> register flow successfully (well, I didn't meet the reserve, but the bid did go through).

I will follow-up on the PR to ask why it was closed and will follow-up here.

@ashfurrow ashfurrow requested a review from sweir27 May 23, 2019 18:06
ashfurrow added a commit that referenced this pull request May 23, 2019
@alloy
Copy link
Contributor

alloy commented May 24, 2019

LGTM, thanks for the fast fix! 🙌

@alloy alloy merged commit 4f76194 into master May 24, 2019
@alloy alloy deleted the ashfurrow-fix-card-infinite-loop branch May 24, 2019 08:46
@ashfurrow
Copy link
Contributor Author

I've heard back from the author of the PR with the changes I've borrowed – they closed the PR because some end-to-end tests weren't working anymore, but that's it. We're going to investigate and compare notes.

@DarkKnight1992
Copy link

I understand the e2e test was broke..... BUT YOU CAN"T JUST CLOSE A PR WITHOUT FIXING THE ISSUE

@ashfurrow
Copy link
Contributor Author

@DarkKnight1992 We didn't close the PR, that's a different repo. I would be happy to provide further clarification on why this solution works for us, but you are unlikely to get assistance from the open source community without a respectful tone.

@DarkKnight1992
Copy link

firstly, i haven't disrespected anyone and sorry for not realizing this was a different repo. And i have a point.

@alloy
Copy link
Contributor

alloy commented Jun 11, 2019

Even if this were the right repo and besides your all-caps tone, which usually doesn’t inspire a fruitful debate–you’re not respecting the prerogative that the author has to close their own PR. They owe you absolutely nothing.

If you wish to get to a place where your problem is solved, which I assume you want, then I would suggest you try a different approach when interacting with the author of the PR. Or you can exercise your right that the MIT license gives–which the authors were so friendly to give to you for free–and apply the change on a fork, like we did in this PR.

Either way, please don’t continue down this line here 🙏

@DarkKnight1992
Copy link

My apologizes didn't mean to offend anybody.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<PaymentCardTextField /> locks up iOS apps on tipsi-stripe v6
3 participants