-
Notifications
You must be signed in to change notification settings - Fork 659
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
Link Wallet menu #9872
base: master
Are you sure you want to change the base?
Link Wallet menu #9872
Conversation
Diffuse output:
APK
|
composable(LinkScreen.CardEdit.ROUTE) { | ||
CardEditScreen() |
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.
The CardEdit
route needs to extract the paymentDetailsId
parameter from the NavBackStackEntry
and pass it to CardEditScreen
. The current implementation drops this required parameter. Consider updating to:
composable(
route = LinkScreen.CardEdit.ROUTE,
arguments = listOf(navArgument("paymentDetailsId") { type = NavType.StringType })
) { backStackEntry ->
val paymentDetailsId = backStackEntry.arguments?.getString("paymentDetailsId")
CardEditScreen(paymentDetailsId = paymentDetailsId)
}
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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.
} | ||
|
||
@Test | ||
fun `viewmodel should open payment method screen when onEditPaymentMethodClicked`() = runTest(dispatcher) { |
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.
The test name appears incorrect - it's testing onAddNewPaymentMethodClicked()
but is named as if it tests onEditPaymentMethodClicked()
. Consider renaming to match the actual behavior being tested.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
2c4a314
to
2316582
Compare
@@ -62,7 +64,12 @@ internal fun WalletScreen( | |||
isExpanded = expanded | |||
}, | |||
onPrimaryButtonClick = viewModel::onPrimaryButtonClicked, | |||
onPayAnotherWayClick = viewModel::onPayAnotherWayClicked, | |||
onPayAnotherWayClicked = viewModel::onPayAnotherWayClicked, | |||
onRemoveClicked = {}, |
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.
The onRemoveClicked
handler is currently empty, preventing payment methods from being removed. This should be wired up to viewModel.onRemovePaymentMethodClicked()
to enable payment method removal functionality.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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.
Summary
Integrate
WalletPaymentMethodMenu
with Link Wallet screenMotivation
JIRA
Testing
Screenshots
Screen.Recording.2025-01-08.at.1.43.06.PM.mov
Changelog