[Identity][Compose] Remove all fragments and use a Compose NavHost #5981
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ccen-stripe
commented
Jan 8, 2023
| import com.stripe.android.identity.analytics.IdentityAnalyticsRequestFactory | ||
| import com.stripe.android.identity.networking.models.Requirement | ||
|
|
||
| internal abstract class IdentityTopLevelDestination( |
There was a problem hiding this comment.
Note: The content of this class is not changed(except the one place below) but just moved from NavigationDestinations.kt
ccen-stripe
commented
Jan 8, 2023
|
|
||
| internal class PassportScanDestination( | ||
| internal abstract class DocumentScanDestination( |
There was a problem hiding this comment.
DocumentScanDestination is the super class of PassportScanDestination, IDScanDestination and DriverLicenseScanDestination, as their parameters list are exactly the same
ccen-stripe
commented
Jan 8, 2023
|
|
||
| internal class PassportUploadDestination( | ||
| internal abstract class DocumentUploadDestination( |
There was a problem hiding this comment.
DocumentUploadDestination is the super class of PassportUploadDestination , IDUploadDestination and DriverLicenseUploadDestination, as their parameters list are exactly the same
|
Diffuse output: APKDEXARSC |
ccen-stripe
commented
Jan 8, 2023
| // If this happens, set the back button destination to [DEFAULT_BACK_BUTTON_DESTINATION] | ||
| const val UNEXPECTED_ROUTE = "UnexpectedRoute" | ||
|
|
||
| fun errorTitle(backStackEntry: NavBackStackEntry) = |
There was a problem hiding this comment.
the parameters passed to a destination is set with destinationRoute.withParams method.
destinationRoute is something like Error?arg1={arg1}&arg2={arg2}
destinationRoute.withParams set the params with actual values Error?arg1=value1&arg2=value2.
This would in turn created a NavBackStackEntry with a bundle with "arg1"="value1", "arg2"="value2" , then we created these static function to fetch corresponding entries. This is a slightly generalized version of FinancialConnection's ManualEntrySuccess.microdeposits
ccen-stripe
commented
Jan 8, 2023
| @@ -53,92 +52,90 @@ internal fun ConfirmationScreen( | |||
| val verificationPageState by identityViewModel.verificationPage.observeAsState(Resource.loading()) | |||
| val context = LocalContext.current | |||
|
|
|||
| MdcTheme { | |||
There was a problem hiding this comment.
Note: all Screen changes are just removing the encapsulating MdcTheme, as we now have it in the NavHost
ccen-stripe
commented
Jan 9, 2023
| * Navigate to the [IdentityTopLevelDestination] by calling NavController.navigate(fragmentID, bundle). | ||
| */ | ||
| internal fun NavController.navigateTo(destination: IdentityTopLevelDestination) { | ||
| navigate(destination.routeWithArgs) { |
There was a problem hiding this comment.
this was changed from navigate(fragmentId) to navigate(routeWithArgs)
awush-stripe
previously approved these changes
Jan 10, 2023
ccen-stripe
force-pushed
the
ccen/composeFinal10
branch
from
26390cd
to
5f7e62d
Compare
Comment on lines
+196
to
+202
| onBackPressedCallback = | ||
| IdentityActivityOnBackPressedCallback( | ||
| this, | ||
| navController, | ||
| identityViewModel | ||
| ) | ||
| onBackPressedDispatcher.addCallback(this, onBackPressedCallback) |
There was a problem hiding this comment.
should this be under a LaunchedEffect? so that it doesn't get retriggered on recompositions.
There was a problem hiding this comment.
it is wrapped within a LaunchedEffect in IdentityNavGraph: link
identity/src/main/java/com/stripe/android/identity/IdentityActivity.kt
Outdated
Show resolved
Hide resolved
| import androidx.navigation.navArgument | ||
| import com.stripe.android.identity.networking.models.CollectedDataParam | ||
|
|
||
| internal class CameraPermissionDeniedDestination( |
There was a problem hiding this comment.
I like this to pass params as an abstraction 👏
| // If this happens, set the back button destination to [DEFAULT_BACK_BUTTON_DESTINATION] | ||
| const val UNEXPECTED_ROUTE = "UnexpectedRoute" | ||
|
|
||
| fun errorTitle(backStackEntry: NavBackStackEntry) = |
identity/src/main/java/com/stripe/android/identity/navigation/IdentityTopLevelDestination.kt
Show resolved
Hide resolved
identity/src/main/java/com/stripe/android/identity/navigation/IdentityTopLevelDestination.kt
Outdated
Show resolved
Hide resolved
| identityViewModel.verificationArgs.brandLogo, | ||
| verificationPage, | ||
| onConsentAgreed = { | ||
| coroutineScope.launch { |
There was a problem hiding this comment.
should this be run using viewModelScope instead? otherwise the api call would be cancelled if the component leaves the composition.
There was a problem hiding this comment.
hmm I think it's OK because postVerificationPageDataAndMaybeNavigate post an API and triggers navigation(which would make ConsentScreen leaving the composition)
carlosmuvi-stripe
approved these changes
Jan 11, 2023
awush-stripe
approved these changes
Jan 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Final PR to fully migrate Identity SDK to jetpack compose.
Before the SDK use a NavController to navigate between
Fragments. This PR replaced all fragments with aIdentityNavGraphwith differentcomposables. Eachcomposableis represented by a String route.Before the PR
IdentityTopLevelDestinationand its subclassesDestinationRouteabstract class, which dynamically populates the route fromrouteBase?argName1={argName1}&argName2={argName2}torouteBase?argName1=value1&argName2=value2, and exposed static method to fetch the arguments fromNavBackStackEntry(similar to FinancialConnection'ManualEntrySuccess.microdepositsmethods), seeErrorDestination.errorTitlefor detailsNavController.navigateTo(destination: IdentityTopLevelDestination), its implementation was to navigate between fragmentIdsBellow is a 1-to-1 mapping between Fragments and subclasses of IdentityTopLevelDestination.
After the PR:
IdentityTopLevelDestinationand their subclasses is moved to their own filesIdentityNavGraphcreated, creating aNavHostwith differentcomposablesNavController.navigateTo(destination: IdentityTopLevelDestination)implementation changed from fragmentId to routesfragmentIds are used as indicator of a screen, now we checked theroutestring insteadNavController.clearDataAndNavigateUp- checking which screen is in backStackEntryIdentityActivity.isConsent/IdentityActivity.isConfirmation/IdentityActivity.isErrorThatShouldFail- checking what is the current screenErrorScreen's backButton destination - indicating which screen to popUp to when back is pressedMdcThemeon individual screens as now we have it in theNavHost, apart from that there are no changes in the screens. These are all the Screen files:ConfirmationScreenConsentScreenDocSelectionScreenDocumentScanScreenErrorScreenSelfieScreenUploadScreenMotivation
Go full Jetpack Compose
Testing
Screenshots
Changelog