Skip to content
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

[connect] Use AndroidX Navigation in Example app #9767

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

lng-stripe
Copy link
Contributor

@lng-stripe lng-stripe commented Dec 9, 2024

Summary

Main changes:

  • Refactor navigation around settings to use the AndroidX Navigation library. Settings is no longer a bottom sheet modal, but a destination on the nav graph. Appearance settings is still a modal to demonstrate real-time updates to the embedded component (otherwise the WebView is reloaded when we nav back)
  • Adds dependency on androidx.hilt:hilt-navigation-compose

See inline comments for more.

Motivation

https://jira.corp.stripe.com/browse/MXMOBILE-2512

This is in preparation for adding Account Onboarding component settings, which we want to navigate to from the Settings screen.

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

In addition to showing the happy path, the demo below also shows error state and app restoration.

Screen.Recording.2024-12-09.at.11.35.36.AM-compressed.mov

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Risky Change

This is considered a risky change because it adjusts the sample app build.gradle, please review carefully.
We've seen issues in the past which resulted in failed builds for merchants. Please make sure the build.gradle change is intended.

By adding the label accept-risky-change to this PR, I acknowledge that I'm changing an example app and have verified that the SDK remains in a shippable state.

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Diffuse output:

OLD: paymentsheet-example-release-master.apk (signature: V1, V2)
NEW: paymentsheet-example-release-pr.apk (signature: V1, V2)

          │          compressed          │        uncompressed        
          ├───────────┬───────────┬──────┼──────────┬──────────┬──────
 APK      │ old       │ new       │ diff │ old      │ new      │ diff 
──────────┼───────────┼───────────┼──────┼──────────┼──────────┼──────
      dex │   3.9 MiB │   3.9 MiB │  0 B │  8.6 MiB │  8.6 MiB │  0 B 
     arsc │   2.3 MiB │   2.3 MiB │  0 B │  2.3 MiB │  2.3 MiB │  0 B 
 manifest │     5 KiB │     5 KiB │  0 B │ 24.9 KiB │ 24.9 KiB │  0 B 
      res │ 908.2 KiB │ 908.2 KiB │  0 B │  1.4 MiB │  1.4 MiB │  0 B 
   native │   2.6 MiB │   2.6 MiB │  0 B │    6 MiB │    6 MiB │  0 B 
    asset │   1.6 MiB │   1.6 MiB │  0 B │  1.6 MiB │  1.6 MiB │  0 B 
    other │   1.4 MiB │   1.4 MiB │ -5 B │  1.6 MiB │  1.6 MiB │  0 B 
──────────┼───────────┼───────────┼──────┼──────────┼──────────┼──────
    total │  12.7 MiB │  12.7 MiB │ -5 B │ 21.6 MiB │ 21.6 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 40570 │ 40570 │ 0 (+0 -0) 
   types │ 14002 │ 14002 │ 0 (+0 -0) 
 classes │ 11679 │ 11679 │ 0 (+0 -0) 
 methods │ 59604 │ 59604 │ 0 (+0 -0) 
  fields │ 39798 │ 39798 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  243 │  243 │  0   
 entries │ 6209 │ 6209 │  0
APK
   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 53.4 KiB │ -5 B │ 118.5 KiB │  0 B │ ∆ META-INF/CERT.SF                        
    272 B │ +2 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
  1.2 KiB │ -2 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA                       
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 54.9 KiB │ -5 B │ 119.9 KiB │  0 B │ (total)

Comment on lines 48 to +51

private enum class SheetType {
SETTINGS,
APPEARANCE,
}
@Suppress("ConstPropertyName")
private object MainDestination {
const val ComponentPicker = "ComponentPicker"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 Ignoring lint and sticking with PascalCase so minimize thrash when we can use the type-safe DSL.

@AndroidEntryPoint
class MainActivity : ComponentActivity() {

private val viewModel: EmbeddedComponentLoaderViewModel by viewModels()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 removed in favor of hiltViewModel() -- see below

* Safer [NavHostController.navigateUp] that prevents navigating past the previous screen,
* typically due to accidental double-clicks.
*/
fun NavHostController.safeNavigateUp() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice extension!

@@ -38,7 +35,7 @@ fun AppearanceView(
navigationIcon = {
IconButton(onClick = onDismiss) {
Icon(
imageVector = Icons.AutoMirrored.Default.ArrowBack,
imageVector = Icons.Default.Close,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 More standard to use X for a modal

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case can we change SettingsView to match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but SettingsView isn't a modal, which can/should be a back arrow

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I see what you're saying. I'm fine keeping it as-is then

@@ -20,14 +20,13 @@ fun BackIconButton(onClick: () -> Unit) {
}

@Composable
fun MoreIconButton(
fun CustomizeAppearanceIconButton(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

@Suppress("LongMethod")
@OptIn(ExperimentalMaterialApi::class)
@Composable
private fun ComponentPickerContent() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 extracted to its own file

@lng-stripe lng-stripe marked this pull request as ready for review December 9, 2024 16:56
@lng-stripe lng-stripe requested review from a team as code owners December 9, 2024 16:56
@lng-stripe lng-stripe added the accept-risky-change accept-risky-change label Dec 9, 2024
@@ -29,6 +29,7 @@ dependencies {
implementation libs.androidx.browser
implementation libs.androidx.fragment
implementation libs.androidx.fragmentCompose
implementation libs.androidx.hiltNavigationCompose
Copy link
Collaborator

@simond-stripe simond-stripe Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to run ./gradlew connectexample:apiDump

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops not that command the one you ran 🤦

@lng-stripe lng-stripe force-pushed the lng/connect-navigation-compose branch from ca5ff6f to 79af50d Compare December 9, 2024 17:56
Copy link
Collaborator

@simond-stripe simond-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! We should have the SettingsView match the AppearanceView though and also use an 'x' in place of a back arrow

* Safer [NavHostController.navigateUp] that prevents navigating past the previous screen,
* typically due to accidental double-clicks.
*/
fun NavHostController.safeNavigateUp() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice extension!

@@ -38,7 +35,7 @@ fun AppearanceView(
navigationIcon = {
IconButton(onClick = onDismiss) {
Icon(
imageVector = Icons.AutoMirrored.Default.ArrowBack,
imageVector = Icons.Default.Close,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case can we change SettingsView to match?

@@ -0,0 +1,9 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Where did you find this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


@OptIn(PrivateBetaConnectSDK::class, ExperimentalMaterialApi::class)
@Composable
fun ComponentPickerContent(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactor!

Copy link
Collaborator

@amk-stripe amk-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gradle file changes LGTM

@lng-stripe lng-stripe merged commit 5d21a09 into master Dec 11, 2024
18 checks passed
@lng-stripe lng-stripe deleted the lng/connect-navigation-compose branch December 11, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accept-risky-change accept-risky-change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants