Separate login UI creation to its own function so that the host app can determine how it is displayed #564
Conversation
…other function `loginUI` so that the caller can determine how to display it.
jaclync
requested review from
Ecarrion,
ScoutHarris,
ctarda and
diegoreymendez
|
This is something we've been discussing for WPiOS with @frosty as well. Pinging him to loop him in. |
| public class func showLogin(from presenter: UIViewController, animated: Bool, showCancel: Bool = false, restrictToWPCom: Bool = false, onLoginButtonTapped: (() -> Void)? = nil, onCompletion: (() -> Void)? = nil) { | ||
| defer { | ||
| trackOpenedLogin() | ||
| guard let loginViewController = loginUI(showCancel: showCancel, restrictToWPCom: restrictToWPCom, onLoginButtonTapped: onLoginButtonTapped) else { |
There was a problem hiding this comment.
So if I understand correctly, this is the method that WordPress would call to initialize the login flow, while WooCommerce would call loginUI?
If that's the case, would it make sense to make the names of both methods a bit more explicit, or rename in a way that the method name indicates that one method presents the login flow while the other leaves the responsibility of presenting the flow to the client app?
I'm curious about what others think though
There was a problem hiding this comment.
@ctarda - My personal opinion is that we'll probably want to follow suit in WPiOS soon (in showing the login UI as the root VC). There are several advantages in doing so.
With this in mind I'd say the naming isn't overly important as we'll eventually deprecate showLogin(...) entirely - the documentation of these methods should be enough for starters.
There was a problem hiding this comment.
Correction: we may never deprecate showLogin(...) as it is currently used in WPiOS to log in to additional sites (which means it wouldn't be shown as the root VC).
Still the naming showLogin(...) and loginUI(...) sounds good to me. The other only option I can think of is to use loginViewController(...) instead of loginUI(...) but I'm not sure that'd make a huge difference.
ctarda
left a comment
ctarda
left a comment
There was a problem hiding this comment.
Thanks for your feedback @diegoreymendez If you don't have any concerns about the naming, I'd say 
|
Thanks for the feedback César and Diego! I tested in WPiOS with this branch wordpress-mobile/WordPress-iOS#15682 and verified the login behavior is as before. Merging the PR now for code freeze next Monday! |
For woocommerce/woocommerce-ios#3107 and woocommerce/woocommerce-ios#3429
Why
When launching the app in logged out state, we call
WordPressAuthenticator.showLoginto show the login UI in both WPiOS and WCiOS apps. However, since iOS 14 we started seeing a delay presenting the login UI (similar WPiOS issue). From my testing, it seems to be due to loading the view controller from Login.storyboard in the authenticator pod which is not easy to change. Because the app window's root view controller is the tab bar UI, the uninitialized tab bar UI is shown briefly before the login UI is presented.In order to solve the glitch in WCiOS, we are setting the login view controller to the app window's root view controller instead of calling
WordPressAuthenticator.showLoginto present the login UI from a source view controller. Therefore, this PR moved the login UI creation to a separate public function so that the host app can decide how it is displayed (set to the app window's root view controller in WCiOS' case).Changes
loginUIwith the same logic from how a login view controller is created and configured from the pre-existingshowLogin.Testing
WPiOS
Please review/test wordpress-mobile/WordPress-iOS#15682 - I think we can merge it once it's approved so that we can also QA the authenticator change in case of regression
WCiOS
Please see the testing section in woocommerce/woocommerce-ios#3498