-
Notifications
You must be signed in to change notification settings - Fork 11
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
Show social login options in site credentials login mode #849
Conversation
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.
This works as expected ✅ I left some comments to improve readability, let me know what you think.
switch screenMode { | ||
case .signInUsingSiteCredentials: | ||
configuration.enableSocialLogin == false | ||
case .signInUsingWordPressComOrSocialAccounts: | ||
configuration.enableSocialLogin == false | ||
} |
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.
Can we get rid of the switch since both cases are the same?
switch screenMode { | |
case .signInUsingSiteCredentials: | |
configuration.enableSocialLogin == false | |
case .signInUsingWordPressComOrSocialAccounts: | |
configuration.enableSocialLogin == false | |
} | |
configuration.enableSocialLogin == false |
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.
We can also remove this bool and update the code as suggested below.
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.
Good point. Removed the switch case in faa401b
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.
We can also remove this bool and update the code as suggested below.
Answered in #849 (comment)
// Add a "Continue" button here as the `continueButton` at the top | ||
// will not be displayed for `signInUsingSiteCredentials` screen mode. | ||
// | ||
buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title, | ||
isPrimary: true, | ||
accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in | ||
self?.handleSubmitButtonTapped() | ||
} | ||
if configuration.enableSocialLogin { | ||
configureSocialButtons() | ||
|
||
// Setup Sign in with site credentials button | ||
buttonViewController.setupBottomButton(attributedTitle: WPStyleGuide.formattedSignInWithSiteCredentialsString(), | ||
isPrimary: false, | ||
accessibilityIdentifier: ButtonConfiguration.SignInWithSiteCredentials.accessibilityIdentifier) { [weak self] in | ||
self?.handleSiteCredentialsButtonTapped() | ||
// Setup Sign in with site credentials button | ||
buttonViewController.setupTertiaryButton(attributedTitle: WPStyleGuide.formattedSignInWithSiteCredentialsString(), | ||
isPrimary: false, | ||
accessibilityIdentifier: ButtonConfiguration.SignInWithSiteCredentials.accessibilityIdentifier) { [weak self] in | ||
self?.handleSiteCredentialsButtonTapped() | ||
} | ||
} else { | ||
// Add a "Continue" button here as the `continueButton` at the top will be hidden | ||
// | ||
if showsContinueButtonAtTheBottom { | ||
buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title, | ||
isPrimary: true, | ||
accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in | ||
self?.handleSubmitButtonTapped() | ||
} | ||
} | ||
|
||
// Setup Sign in with site credentials button | ||
buttonViewController.setupBottomButton(attributedTitle: WPStyleGuide.formattedSignInWithSiteCredentialsString(), | ||
isPrimary: false, | ||
accessibilityIdentifier: ButtonConfiguration.SignInWithSiteCredentials.accessibilityIdentifier) { [weak self] in | ||
self?.handleSiteCredentialsButtonTapped() | ||
} |
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.
Maybe we can simplify this for readability:
if configuration.enableSocialLogin {
configureSocialButtons()
} else {
buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title,
isPrimary: true,
accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in
self?.handleSubmitButtonTapped()
}
}
// Setup Sign in with site credentials button
buttonViewController.setupBottomButton(attributedTitle: WPStyleGuide.formattedSignInWithSiteCredentialsString(),
isPrimary: false,
accessibilityIdentifier: ButtonConfiguration.SignInWithSiteCredentials.accessibilityIdentifier) { [weak self] in
self?.handleSiteCredentialsButtonTapped()
}
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.
The site credentials button needs to be set in the tertiary button when social login is enabled in config. (It is not always the bottom button) So the above code won't work.
Also, keeping the showsContinueButtonAtTheBottom
boolean helps to maintain code readability in other places.
Thanks for the review, @itsmeichigo! 🙇 I have addressed your comments. Please feel free to comment if you have further comments. |
All good, please feel free to merge the PR. |
Closes #848
Description
The login screen
GetStartedViewController
doesn't show the social login buttons ifenableSiteCredentialsLoginForSelfHostedSites
is enabled in configuration.The social buttons should be displayed if
enableSocialLogin
is enabled in the configuration. (Irrespective of whetherenableSiteCredentialsLoginForSelfHostedSites
is true/false)This PR updates the
GetStartedViewController
to show the social login options.Warnings in CI
There are warnings while validating the podspec because a method related to Gravatar image has been deprecated in wordpress-mobile/WordPressUI-iOS#156
I made an attempt to resolve the warnings.
For the time being, I used #850 by Tony, which allows warnings in CI.
Internal discussion about why allow-warnings has been enabled in CI - p1712640270467459/1712574752.341039-slack-CC7L49W13
Testing
Please follow the steps from woocommerce/woocommerce-ios#12429 for testing.
CHANGELOG.md
if necessary.