-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rank providers by usability when picking top provider #1362
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
Rank providers by usability when picking top provider #1362
Conversation
As a side effect, this fixes the bug where linking is broken if the developer removes support for a provider. Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX do you think developers expect what they passed in to to be a priority? In my opinion it's just a UI concern, devs may be choosing this order based on color combinations / other aesthetic concerns. Also is it safe (with linking and stuff) to change this ordering now? |
@samtstern Huh, I hadn't thought of it that way. Basically, my gripe is that if I'm already linked to Google and then add Facebook, we're always going to try and use Facebook—even if I sign in later with Google again. And we all know that no sign-in method can compete with Google's simplicity. That's why we stopped showing the others before the picker screen. I'd be happy to just force Google to be on top, but I feel like we had decided against that for some reason. What do you think? |
@SUPERCILEX I do see what you're saying but I think we want a way to solve this that's not tied to the order things are presented in the UI ... let's punt to 4.2.0 and think about it more. |
SGTM! |
…er-priority # Conflicts: # auth/src/main/java/com/firebase/ui/auth/util/data/ProviderUtils.java # auth/src/main/java/com/firebase/ui/auth/viewmodel/email/EmailProviderResponseHandler.java # auth/src/main/java/com/firebase/ui/auth/viewmodel/idp/SocialProviderResponseHandler.java # auth/src/test/java/com/firebase/ui/auth/AuthUITest.java
Signed-off-by: Alex Saveau <[email protected]>
@samtstern Alrighty, I redid this PR so it has these changes now:
WDYT? |
Signed-off-by: Alex Saveau <[email protected]>
Great |
@samtstern WDYT? |
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.
If the Google provider is present, make it the priority. I think this is fair not only because Firebase is a Google product, but also because Google sign-in is objectively a way better experience for the user than any other provider.
I would prefer to remove this and not be as opinionated. I know that the Firebase backend already prefers Google in some cases but in my experience developers don't really like that! Let's just allow the providers to go through in order, wdyt?
} | ||
} | ||
|
||
if (lastSignedInProviders.contains(GoogleAuthProvider.PROVIDER_ID)) { |
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.
Add a comment here explaining what you're doing (making sure Google is always first) and why?
startWelcomeBackFlowForLinking( | ||
ProviderUtils.getTopProvider(providers), | ||
response); | ||
if (providers.isEmpty()) { |
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.
nit: instead of else { if {} else {} }
how about else if { } else { }
?
(also apologies for being so slow to get to this! I like the rewrite though) |
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
@samtstern The reasoning behind this change is that the Google provider is simply a better experience for the user (less clicks). Devs complain about other providers being removed in favor of Google (which isn't great), but this is only a user-facing change. All this does is make it so users get the 1-click experience when signing back in (and doesn't change the sign-up experience). Is that reasonable? |
Great idea |
@SUPERCILEX ok I am convinced, your judgement on these things is almost always good. LGTM. |
As a side effect, this fixes the bug where linking is broken if the developer removes support for a provider.