-
Notifications
You must be signed in to change notification settings - Fork 263
Migrate sample to compose #173
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
base: main
Are you sure you want to change the base?
Conversation
/gemini review |
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.
Code Review
This pull request successfully migrates the CredentialManager sample app from the traditional View system (Fragments and XML) to Jetpack Compose. The migration is comprehensive, replacing all UI components, navigation, and adapting the logic to a ViewModel-based architecture. The code is now more modern and follows many Compose best practices.
However, there are a few significant areas for improvement. The most critical is the downgrading of several key AndroidX and Kotlin libraries in build.gradle
, which is a regression and should be reverted. Additionally, there are architectural concerns, primarily passing Activity
and Context
instances into ViewModels, which violates MVVM principles and can lead to memory leaks. I've also left comments on improving navigation logic, state handling, and making Compose Previews more robust.
Overall, this is a great step forward for the sample app. Addressing the feedback will make it an even better example of a modern Android application.
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignInScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignInViewModel.kt
Outdated
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignUpScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignUpScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/HomeScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/HomeScreen.kt
Outdated
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignInScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignInScreen.kt
Outdated
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignUpScreen.kt
Outdated
Show resolved
Hide resolved
…ng factories for the view models and instantiating credman through lambda functions passed to the screens.
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.
Hi Chiping, Kindly run Spotless to have copyright, formating and javadocs consitent. WIll review once you are done to focus on other stuff :)
No description provided.