LinkActivity, ViewModel and Navigation #9334
Conversation
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
2 times, most recently
from
27fc249 to
04bd804
Compare
|
Diffuse output: APKMANIFESTDEX |
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
2 times, most recently
from
f4375bd to
fdb91ec
Compare
toluo-stripe
changed the title
toluo-stripe
changed the title
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
from
fdb91ec to
34958ca
Compare
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
from
34958ca to
0a05ef7
Compare
| import com.stripe.android.link.ui.verification.VerificationScreen | ||
| import com.stripe.android.link.ui.wallet.WalletScreen | ||
|
|
||
| internal class LinkActivity : AppCompatActivity() { |
There was a problem hiding this comment.
What will navigation between PaymentSheet and LinkActivity look like? Since this is its own activity with its own navigation, I'm assuming this will be totally separate but I'm curious what the transition will look like
There was a problem hiding this comment.
Yh, it'll be separate from PaymentSheet. PaymentSheet will launch this with an activityResultCaller.
| initialState = LinkState, | ||
| ) { | ||
| override fun actionToResult(action: LinkAction): Flow<LinkResult> { | ||
| // These are placeholder actions, they may be removed |
There was a problem hiding this comment.
what does this comment mean? If we don't need these actions, I don't think we should merge them in
|
|
||
| // This will be used as a base for all Link screen viewmodels to create a consistent architecture | ||
| // There should be no more logic here besides updating state and sending effects | ||
| internal abstract class LinkViewModel<State, Action, Result, Effect>( |
There was a problem hiding this comment.
what's the advantage of having this as a superclass to the view models, rather than having fully independent interactors, like we have for PaymentSheetScreen?
There was a problem hiding this comment.
Following up on this: I think we should prefer [composition over inheritance](https://en.wikipedia.org/wiki/Composition_over_inheritance#:~:text=Composition%20over%20inheritance%20(or%20composite,from%20a%20base%20or%20parent) if you'd like to re-use the function to covert actions -> results -> state/effects
| import kotlinx.coroutines.flow.update | ||
| import kotlinx.coroutines.launch | ||
|
|
||
| // This will be used as a base for all Link screen viewmodels to create a consistent architecture |
There was a problem hiding this comment.
these comments are helpful for review! I think these work better as github comments for review only, and shouldn't be checked in. If you want to check in comments like this, you should add proper javadoc.
|
|
||
| // This will be used as a base for all Link screen viewmodels to create a consistent architecture | ||
| // There should be no more logic here besides updating state and sending effects | ||
| internal abstract class LinkViewModel<State, Action, Result, Effect>( |
There was a problem hiding this comment.
Following up on this: I think we should prefer [composition over inheritance](https://en.wikipedia.org/wiki/Composition_over_inheritance#:~:text=Composition%20over%20inheritance%20(or%20composite,from%20a%20base%20or%20parent) if you'd like to re-use the function to covert actions -> results -> state/effects
|
|
||
| sealed interface SignUpResult { | ||
| data object ShowLoader : SignUpResult | ||
| data class SendEffect(val effect: SignUpEffect) : SignUpResult |
There was a problem hiding this comment.
I think having a result called SendEffect is conflating results with effects. It makes more sense to me for this to be something like SignUpSucceeded or SignUpFailed. Then the resultToEffect function would be responsible for determining that e.g. if sign up succeeds, then we should navigate to the wallet screen
| LaunchedEffect("SignUpEffects") { | ||
| viewModel.effect.collect { effect -> | ||
| when (effect) { | ||
| SignUpEffect.NavigateToWallet -> navController.navigate(LinkScreen.Wallet.route) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to navigate to the wallet screen should be in the view model, not in the UI
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
from
5181e6a to
dbc537d
Compare
| } | ||
|
|
||
| private fun handleBackPressed() { | ||
| dismissWithResult?.invoke(LinkActivityResult.Canceled()) |
There was a problem hiding this comment.
I think this should be called something else. handleBackPressed sounds like it's for navigating back between screens, and then maybe exiting the whole activity if the back stack is empty. But the implementation looks like it just cancels.
There was a problem hiding this comment.
I was going to leave this for when I start implementing screens, but it makes sense to just add the back navigation now
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
from
79c1e42 to
7f65fd9
Compare
toluo-stripe
force-pushed
the
tolu/link-navigator
branch
from
7f65fd9 to
fe8b66e
Compare
Summary
Link Activity, Navigator and ViewModel
I mentioned that we will use typed-navigation here, but that requires upgrading to compose 1.6 . Since we can't upgrade, I decided to add string routes to
LinkScreenMotivation
JIRA
Testing