-
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
Changes from 12 commits
cebe7ec
eea31e3
c973004
1d7b12c
a0bb123
f8323c6
7122945
59bc040
e8a678f
1e0b306
fe43e9b
cd415e8
faa401b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,8 +124,12 @@ class GetStartedViewController: LoginViewController, NUXKeyboardResponder { | |
}() | ||
|
||
private var showsContinueButtonAtTheBottom: Bool { | ||
screenMode == .signInUsingSiteCredentials || | ||
switch screenMode { | ||
case .signInUsingSiteCredentials: | ||
configuration.enableSocialLogin == false | ||
case .signInUsingWordPressComOrSocialAccounts: | ||
configuration.enableSocialLogin == false | ||
} | ||
} | ||
|
||
override open var sourceTag: WordPressSupportSourceTag { | ||
|
@@ -811,20 +815,32 @@ private extension GetStartedViewController { | |
|
||
buttonViewController.hideShadowView() | ||
|
||
// 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 commentThe 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 commentThe 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 |
||
} | ||
} | ||
|
||
|
@@ -835,12 +851,14 @@ private extension GetStartedViewController { | |
|
||
buttonViewController.hideShadowView() | ||
|
||
// Add a "Continue" button here as the `continueButton` at the top will be hidden | ||
// | ||
buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title, | ||
isPrimary: true, | ||
accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in | ||
self?.handleSubmitButtonTapped() | ||
if showsContinueButtonAtTheBottom { | ||
// Add a "Continue" button here as the `continueButton` at the top will be hidden | ||
// | ||
buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title, | ||
isPrimary: true, | ||
accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in | ||
self?.handleSubmitButtonTapped() | ||
} | ||
} | ||
} | ||
|
||
|
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?
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.
Answered in #849 (comment)