-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: move from SmartLock to CredentialManager #2180
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
feat: move from SmartLock to CredentialManager #2180
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.
Sorry for the delayed review!
google() | ||
mavenCentral() | ||
mavenLocal() | ||
maven("https://oss.jfrog.org/artifactory/oss-release-local") |
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 assume we use this repo for build-info-extractor-gradle
?
I have no idea why we use that plugin, can we maybe add a comment here to say that this line is needed for that plugin? That way we removed them altogether if we find out that it is not needed.
|
||
viewModelScope.launch { | ||
try { | ||
credentialManager.createCredential(activity, request) |
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.
Looks like createCredential()
takes Context
as argument. Maybe we can pass that to saveCredentials()
instead of activity: androidx.activity.ComponentActivity
?
Also, do we care about the result of createCredential()
? it seems like we're ignoring it right now.
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.
From what I understood, we don't need to get the result - if createCredential()
succeeds, we've safely saved the user's password. But that brings me to another question: (out of curiosity) what's in the result
of this call?
auth/src/main/java/com/firebase/ui/auth/ui/credentials/CredentialSaveActivity.kt
Outdated
Show resolved
Hide resolved
} else { | ||
startAuthMethodChoice(); | ||
} | ||
startAuthMethodChoice(); |
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.
Some of the code we deleted here seems to be related to emailLink
- do we want to keep that?
auth/src/main/java/com/firebase/ui/auth/viewmodel/SignInViewModelBase.java
Show resolved
Hide resolved
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
public class KickoffActivity extends InvisibleActivityBase { | ||
private SignInKickstarter mKickstarter; | ||
public static SignInKickstarter mKickstarter; |
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.
Keeping a reference to a static object in Android is a really bad idea, but I see what you meant when you said that state management is hard when we move stuff from KickoffActivity to AuthMethodPicker - I'll let this be for now, and we can change in the future if we re-architect this flow.
Hey there! So you want to contribute to FirebaseUI? Before you file this pull request, follow these steps:
./gradlew check
to ensure the Travis build passes.