-
Notifications
You must be signed in to change notification settings - Fork 649
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
[Financial Connections] Adds Mavericks to handle state-based UIs. #4986
Changes from 9 commits
e16f3be
1330e6d
b09df88
8cfa170
6e079b3
3d62e47
d715525
bd7005e
86c1ac5
1077771
d55708d
72ee4d4
fd85581
da1207b
041e91a
8b531d2
e5e6fbc
ea7d683
aaa6b24
c80ac9d
d116c39
4922ae7
62b7edc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,8 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:app="http://schemas.android.com/apk/res-auto" | ||
<androidx.fragment.app.FragmentContainerView xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:tools="http://schemas.android.com/tools" | ||
android:id="@+id/nav_host_fragment" | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent"> | ||
|
||
<com.google.android.material.progressindicator.CircularProgressIndicator | ||
android:id="@+id/spinner" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:indeterminate="true" | ||
app:indicatorColor="@color/stripe_toolbar_color_default" | ||
app:indicatorSize="@dimen/stripe_connectionssheet_loading_indicator_size" | ||
app:layout_constraintBottom_toBottomOf="parent" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" | ||
app:trackThickness="@dimen/stripe_connectionssheet_loading_indicator_stroke_width" /> | ||
|
||
</androidx.constraintlayout.widget.ConstraintLayout> | ||
android:layout_height="match_parent" | ||
android:orientation="vertical" | ||
tools:context=".FinancialConnectionsSheetActivity" /> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:app="http://schemas.android.com/apk/res-auto" | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent"> | ||
|
||
<com.google.android.material.progressindicator.CircularProgressIndicator | ||
android:id="@+id/spinner" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_gravity="center" | ||
android:indeterminate="true" | ||
app:indicatorColor="@color/stripe_toolbar_color_default" | ||
app:indicatorSize="@dimen/stripe_connectionssheet_loading_indicator_size" | ||
app:layout_constraintBottom_toBottomOf="parent" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" | ||
app:trackThickness="@dimen/stripe_connectionssheet_loading_indicator_stroke_width" /> | ||
|
||
</FrameLayout> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
package com.stripe.android.financialconnections | ||
|
||
import android.app.Activity | ||
import android.content.Intent | ||
import android.net.Uri | ||
import android.os.Bundle | ||
import androidx.activity.addCallback | ||
import androidx.activity.result.contract.ActivityResultContracts.StartActivityForResult | ||
import androidx.fragment.app.Fragment | ||
import com.airbnb.mvrx.MavericksView | ||
import com.airbnb.mvrx.activityViewModel | ||
import com.airbnb.mvrx.withState | ||
import com.stripe.android.financialconnections.FinancialConnectionsSheetViewEffect.FinishWithResult | ||
import com.stripe.android.financialconnections.FinancialConnectionsSheetViewEffect.OpenAuthFlowWithUrl | ||
import com.stripe.android.financialconnections.launcher.FinancialConnectionsSheetActivityResult | ||
import com.stripe.android.financialconnections.presentation.CreateBrowserIntentForUrl | ||
|
||
internal class FinancialConnectionsSheetFragment : | ||
Fragment(R.layout.fragment_financial_connections_sheet), MavericksView { | ||
|
||
private val startForResult = registerForActivityResult(StartActivityForResult()) { | ||
viewModel.onActivityResult() | ||
} | ||
|
||
private val viewModel: FinancialConnectionsSheetViewModel by activityViewModel() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need an activityViewModel to receive the |
||
|
||
override fun onCreate(savedInstanceState: Bundle?) { | ||
super.onCreate(savedInstanceState) | ||
requireActivity().onBackPressedDispatcher.addCallback(this) { | ||
finishWithResult(FinancialConnectionsSheetActivityResult.Canceled) | ||
} | ||
} | ||
|
||
/** | ||
* handle state changes here. | ||
*/ | ||
override fun invalidate() { | ||
withState(viewModel) { state -> | ||
if (state.viewEffect != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use |
||
when (state.viewEffect) { | ||
is OpenAuthFlowWithUrl -> state.viewEffect.launch() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can directly define the launch() function inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean by adding a each viewEffect uses different dependencies hosted by the fragment, so we can't have a unified |
||
is FinishWithResult -> finishWithResult(state.viewEffect.result) | ||
} | ||
viewModel.onViewEffectLaunched() | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When state changes, |
||
|
||
private fun OpenAuthFlowWithUrl.launch() { | ||
val uri = Uri.parse(this.url) | ||
startForResult.launch( | ||
CreateBrowserIntentForUrl( | ||
context = requireContext(), | ||
uri = uri, | ||
) | ||
) | ||
} | ||
|
||
override fun onResume() { | ||
super.onResume() | ||
viewModel.onResume() | ||
} | ||
|
||
private fun finishWithResult(result: FinancialConnectionsSheetActivityResult) { | ||
requireActivity().setResult( | ||
Activity.RESULT_OK, | ||
Intent().putExtras(result.toBundle()) | ||
) | ||
requireActivity().finish() | ||
} | ||
} |
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.
since there's only one fragment, is it possible to declare it deirectly in xml?
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.
Also why do we change the Activity to Fragment? Is it required by Mavericks?
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.
It is supported, but they recommend using fragments, as "Putting UI directly inside of an Activity is no longer recommended in general for Android." (airbnb/mavericks#616 (comment))
However if Compose will be used to build the UI, the activity is still a recommended approach - https://github.com/airbnb/mavericks/blob/main/sample-compose/src/main/java/com/airbnb/mvrx/compose/sample/ComposeSampleActivity.kt so the fragment would be removed in that case.
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.
Actually, rethinking about it, the fragment doesn't bring any value on this case as there's no UI currently. Moved things back to the activity - aaa6b24