-
Notifications
You must be signed in to change notification settings - Fork 120
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
Doesn't work on a presented ViewController (Modal) #168
Conversation
Fix the way we show accountkit navigation controller: Old way's issues: If we call AccountKit from a presentedViewController (Modal), it won't work. Error log: Warning: Attempt to present <AKFNavigationController: 0x107000000> on <UIViewController: 0x1061398d0> which is already presenting <RCTModalHostViewController: 0x1060d0bc0> Seems like the App's current ViewController is busy presenting the modal which wants to present AccountKit.
ios/RNAccountKitViewController.m
Outdated
@@ -58,7 +58,7 @@ - (void)loginWithPhone: (RCTPromiseResolveBlock)resolve | |||
UIViewController<AKFViewController> *viewController = [_accountKit viewControllerForPhoneLoginWithPhoneNumber:prefillPhoneNumber state:inputState]; | |||
[self _prepareLoginViewController:viewController]; | |||
UIViewController *rootViewController = [UIApplication sharedApplication].delegate.window.rootViewController; | |||
[rootViewController presentViewController:viewController animated:YES completion:NULL]; | |||
[rootViewController showViewController:viewController sender:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if this can be configurable. I think two options here:
- Adding a new field to the
RNAccountKit.configure
argument:
RNAccountKit.configure({
...
viewControllerMode: 'show'|'present' // for iOS only, 'present' by default
})
- Changing the signature of
loginWithPhone
andloginWithEmail
only if we can keep the method backward compatible (It could be more difficult to implement.
RNAccountKit.loginWithEmail({
viewControllerMode: 'show'|'present' // 'present' by default
})
RNAccountKit.loginWithPhone({
viewControllerMode: 'show'|'present' // 'present' by default
})
``
Adding @gaguirre to the discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your guide, I will try then back to you 👍
I've updated my changes to support more options while configuring RNAccountKit. Could you please review it and give me some feedback? |
@@ -29,6 +29,7 @@ class RNAccountKit { | |||
readPhoneStateEnabled: true, | |||
receiveSMS: true, | |||
theme: {}, | |||
viewControllerMode: 'present' // for iOS only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have this option included in the Readme
Also you can include a note maybe with the link of the issue it solves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated README, could you please take a look?
ios/RNAccountKitViewController.m
Outdated
[rootViewController showViewController:viewController sender:nil]; | ||
|
||
if([_viewControllerMode isEqualToString:@"present"]) { | ||
[rootViewController showDetailViewController:viewController sender:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be this line as it was before? is there a reason to change that?
[rootViewController presentViewController:viewController animated:YES completion:NULL];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, I reverted it to the old way.
ios/RNAccountKitViewController.m
Outdated
[rootViewController showViewController:viewController sender:nil]; | ||
|
||
if([_viewControllerMode isEqualToString:@"present"]) { | ||
[rootViewController showDetailViewController:viewController sender:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
Guys, I've just published |
Thanks @jpgarcia ! |
Hi @jpgarcia ,
there is nothing happen :(
|
@danhnguyeen |
Fix the way we show accountkit navigation controller:
Old way's issues: If we call AccountKit from a presentedViewController (Modal), it won't work. Error log:
Warning: Attempt to present <AKFNavigationController: 0x107000000> on <UIViewController: 0x1061398d0> which is already presenting <RCTModalHostViewController: 0x1060d0bc0>
Seems like the App's current ViewController is busy presenting the modal which wants to present AccountKit.