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

Fix several UX issues in address element #5462

Merged
merged 2 commits into from Aug 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -12,6 +12,7 @@ import com.google.android.gms.wallet.AutoResolveHelper
import com.google.android.gms.wallet.PaymentData
import com.stripe.android.model.PaymentMethodCreateParams
import com.stripe.android.model.StripeIntent
import com.stripe.android.utils.AnimationConstants
import com.stripe.android.view.AuthActivityStarterHost
import kotlinx.coroutines.launch
import org.json.JSONObject
Expand Down Expand Up @@ -46,7 +47,7 @@ internal class GooglePayLauncherActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

disableAnimations()
setFadeAnimations()

args = runCatching {
requireNotNull(GooglePayLauncherContract.Args.fromIntent(intent)) {
Expand Down Expand Up @@ -89,7 +90,7 @@ internal class GooglePayLauncherActivity : AppCompatActivity() {

override fun finish() {
super.finish()
disableAnimations()
setFadeAnimations()
}

private fun payWithGoogle(task: Task<PaymentData>) {
Expand Down Expand Up @@ -179,9 +180,8 @@ internal class GooglePayLauncherActivity : AppCompatActivity() {
finish()
}

private fun disableAnimations() {
// this is a transparent Activity so we want to disable animations
overridePendingTransition(0, 0)
private fun setFadeAnimations() {
overridePendingTransition(AnimationConstants.FADE_IN, AnimationConstants.FADE_OUT)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I figured we could use fade animations in all places where we were currently overriding pending transitions, as this results in a much smoother look. Any concerns about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it change anything? This activity has no UI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That one is indeed a bad example 🙈 There is a noticeable difference with the payment sheet though. I figured it wouldn’t hurt to apply the same everywhere to discourage using no animations.

}

private companion object {
Expand Down
Expand Up @@ -11,6 +11,7 @@ import com.google.android.gms.tasks.Task
import com.google.android.gms.wallet.AutoResolveHelper
import com.google.android.gms.wallet.PaymentData
import com.stripe.android.model.PaymentMethod
import com.stripe.android.utils.AnimationConstants
import kotlinx.coroutines.launch

/**
Expand Down Expand Up @@ -46,7 +47,7 @@ internal class GooglePayPaymentMethodLauncherActivity : AppCompatActivity() {
window.statusBarColor = statusColor
}

disableAnimations()
setFadeAnimations()

val nullableArgs = GooglePayPaymentMethodLauncherContract.Args.fromIntent(intent)
if (nullableArgs == null) {
Expand Down Expand Up @@ -90,7 +91,7 @@ internal class GooglePayPaymentMethodLauncherActivity : AppCompatActivity() {

override fun finish() {
super.finish()
disableAnimations()
setFadeAnimations()
}

private fun launchGooglePay(task: Task<PaymentData>) {
Expand Down Expand Up @@ -174,9 +175,8 @@ internal class GooglePayPaymentMethodLauncherActivity : AppCompatActivity() {
finish()
}

private fun disableAnimations() {
// this is a transparent Activity so we want to disable animations
overridePendingTransition(0, 0)
private fun setFadeAnimations() {
overridePendingTransition(AnimationConstants.FADE_IN, AnimationConstants.FADE_OUT)
}

private fun googlePayStatusCodeToErrorCode(googlePayStatusCode: Int):
Expand Down
Expand Up @@ -13,6 +13,7 @@ import com.stripe.android.model.GooglePayResult
import com.stripe.android.model.PaymentMethod
import com.stripe.android.model.PaymentMethodCreateParams
import com.stripe.android.model.ShippingInformation
import com.stripe.android.utils.AnimationConstants
import org.json.JSONObject

/**
Expand Down Expand Up @@ -48,7 +49,7 @@ internal class StripeGooglePayActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

overridePendingTransition(0, 0)
setFadeAnimations()
setResult(
RESULT_OK,
Intent().putExtras(
Expand Down Expand Up @@ -88,7 +89,11 @@ internal class StripeGooglePayActivity : AppCompatActivity() {

override fun finish() {
super.finish()
overridePendingTransition(0, 0)
setFadeAnimations()
}

private fun setFadeAnimations() {
overridePendingTransition(AnimationConstants.FADE_IN, AnimationConstants.FADE_OUT)
}

/**
Expand Down
Expand Up @@ -8,6 +8,7 @@ import androidx.activity.viewModels
import androidx.annotation.VisibleForTesting
import androidx.appcompat.app.AppCompatActivity
import androidx.lifecycle.ViewModelProvider
import com.stripe.android.utils.AnimationConstants
import com.stripe.android.view.AuthActivityStarterHost

/**
Expand Down Expand Up @@ -35,7 +36,7 @@ internal class PaymentLauncherConfirmationActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

disableAnimations()
setFadeAnimations()

val args = runCatching {
requireNotNull(starterArgs) {
Expand Down Expand Up @@ -73,12 +74,11 @@ internal class PaymentLauncherConfirmationActivity : AppCompatActivity() {

override fun finish() {
super.finish()
disableAnimations()
setFadeAnimations()
}

private fun disableAnimations() {
// this is a transparent Activity so we want to disable animations
overridePendingTransition(0, 0)
private fun setFadeAnimations() {
overridePendingTransition(AnimationConstants.FADE_IN, AnimationConstants.FADE_OUT)
}

/**
Expand Down
@@ -1,9 +1,11 @@
package com.stripe.android.paymentsheet.ui
package com.stripe.android.utils

import androidx.annotation.AnimRes
import com.stripe.android.paymentsheet.R
import androidx.annotation.RestrictTo
import com.stripe.android.R

internal object AnimationConstants {
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
object AnimationConstants {
@AnimRes
val FADE_IN = R.anim.stripe_paymentsheet_transition_fade_in

Expand Down
8 changes: 0 additions & 8 deletions paymentsheet/api/paymentsheet.api
Expand Up @@ -855,14 +855,6 @@ public final class com/stripe/android/paymentsheet/addresselement/AddressLaunche
public synthetic fun newArray (I)[Ljava/lang/Object;
}

public final class com/stripe/android/paymentsheet/addresselement/AddressUtilsKt {
public static final fun levenshtein (Ljava/lang/CharSequence;Ljava/lang/CharSequence;)I
}

public final class com/stripe/android/paymentsheet/addresselement/AutocompleteScreenKt {
public static final field TEST_TAG_ATTRIBUTION_DRAWABLE Ljava/lang/String;
}
Comment on lines -858 to -864
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two things were made public by accident, right?


public final class com/stripe/android/paymentsheet/addresselement/AutocompleteViewModel_Factory : dagger/internal/Factory {
public fun <init> (Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;)V
public static fun create (Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;Ljavax/inject/Provider;)Lcom/stripe/android/paymentsheet/addresselement/AutocompleteViewModel_Factory;
Expand Down
Expand Up @@ -29,7 +29,7 @@ import com.stripe.android.paymentsheet.model.getPMAddForm
import com.stripe.android.paymentsheet.paymentdatacollection.ComposeFormDataCollectionFragment
import com.stripe.android.paymentsheet.paymentdatacollection.FormFragmentArguments
import com.stripe.android.paymentsheet.paymentdatacollection.ach.USBankAccountFormFragment
import com.stripe.android.paymentsheet.ui.AnimationConstants
import com.stripe.android.utils.AnimationConstants
import com.stripe.android.paymentsheet.ui.PrimaryButton
import com.stripe.android.paymentsheet.viewmodels.BaseSheetViewModel
import com.stripe.android.ui.core.Amount
Expand Down
Expand Up @@ -21,7 +21,7 @@ import androidx.lifecycle.ViewModelProvider
import com.google.android.material.appbar.AppBarLayout
import com.google.android.material.appbar.MaterialToolbar
import com.stripe.android.paymentsheet.databinding.ActivityPaymentOptionsBinding
import com.stripe.android.paymentsheet.ui.AnimationConstants
import com.stripe.android.utils.AnimationConstants
import com.stripe.android.paymentsheet.ui.BaseSheetActivity
import com.stripe.android.paymentsheet.ui.PrimaryButton
import com.stripe.android.paymentsheet.viewmodels.BaseSheetViewModel
Expand Down
Expand Up @@ -27,7 +27,7 @@ import com.stripe.android.paymentsheet.PaymentSheetViewModel.CheckoutIdentifier
import com.stripe.android.paymentsheet.databinding.ActivityPaymentSheetBinding
import com.stripe.android.paymentsheet.model.PaymentSelection
import com.stripe.android.paymentsheet.model.PaymentSheetViewState
import com.stripe.android.paymentsheet.ui.AnimationConstants
import com.stripe.android.utils.AnimationConstants
import com.stripe.android.paymentsheet.ui.BaseSheetActivity
import com.stripe.android.paymentsheet.ui.GooglePayDividerUi
import com.stripe.android.paymentsheet.ui.PrimaryButton
Expand Down
Expand Up @@ -29,7 +29,9 @@ import com.google.accompanist.navigation.animation.composable
import com.google.accompanist.navigation.animation.AnimatedNavHost
import com.google.accompanist.navigation.animation.rememberAnimatedNavController
import com.stripe.android.paymentsheet.parseAppearance
import com.stripe.android.utils.AnimationConstants
import com.stripe.android.ui.core.PaymentsTheme
import kotlinx.coroutines.flow.drop
import kotlinx.coroutines.launch

@OptIn(ExperimentalMaterialApi::class)
Expand Down Expand Up @@ -57,22 +59,36 @@ internal class AddressElementActivity : ComponentActivity() {
WindowCompat.setDecorFitsSystemWindows(window, false)
starterArgs.config?.appearance?.parseAppearance()

starterArgs.statusBarColor?.let {
window.statusBarColor = it
}

// set a default result in case the user closes the sheet manually
setResult()

setContent {
val modalBottomSheetState =
rememberModalBottomSheetState(
ModalBottomSheetValue.Expanded
)
val modalBottomSheetState = rememberModalBottomSheetState(
initialValue = ModalBottomSheetValue.Hidden,
skipHalfExpanded = true,
confirmStateChange = {
val route = navController.currentDestination?.route
route != AddressElementScreen.Autocomplete.route
}
Comment on lines +73 to +76
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find that the user shouldn’t be able to exit from the sheet while in the autocomplete screen. Any concerns around this?

)

navController = rememberAnimatedNavController()
viewModel.navigator.navigationController = navController

val coroutineScope = rememberCoroutineScope()

LaunchedEffect(Unit) {
snapshotFlow { modalBottomSheetState.currentValue }.collect {
modalBottomSheetState.show()
}

LaunchedEffect(Unit) {
// We need to drop(1) to avoid the sheet being closed on the first composition,
// given that the initial bottom sheet state is `hidden`.
snapshotFlow { modalBottomSheetState.currentValue }.drop(1).collect {
// finish the activity when the sheet closes.
if (it == ModalBottomSheetValue.Hidden) {
finish()
Expand Down Expand Up @@ -125,7 +141,9 @@ internal class AddressElementActivity : ComponentActivity() {
}
},
content = {},
modifier = Modifier.navigationBarsPadding().systemBarsPadding()
modifier = Modifier
.navigationBarsPadding()
.systemBarsPadding()
)
}
}
Expand All @@ -138,4 +156,9 @@ internal class AddressElementActivity : ComponentActivity() {
)
)
}

override fun finish() {
super.finish()
overridePendingTransition(AnimationConstants.FADE_IN, AnimationConstants.FADE_OUT)
}
}
@@ -1,8 +1,10 @@
package com.stripe.android.paymentsheet.addresselement

import android.app.Activity
import android.content.Context
import android.content.Intent
import androidx.activity.result.contract.ActivityResultContract
import androidx.annotation.ColorInt
import androidx.annotation.RestrictTo
import androidx.core.os.bundleOf
import com.stripe.android.core.injection.DUMMY_INJECTOR_KEY
Expand All @@ -14,9 +16,11 @@ import kotlinx.parcelize.Parcelize
internal class AddressElementActivityContract :
ActivityResultContract<AddressElementActivityContract.Args, AddressLauncherResult>() {

override fun createIntent(context: Context, input: Args) =
Intent(context, AddressElementActivity::class.java)
.putExtra(EXTRA_ARGS, input)
override fun createIntent(context: Context, input: Args): Intent {
val statusBarColor = (context as? Activity)?.window?.statusBarColor
return Intent(context, AddressElementActivity::class.java)
.putExtra(EXTRA_ARGS, input.copy(statusBarColor = statusBarColor))
}

override fun parseResult(resultCode: Int, intent: Intent?) =
intent?.getParcelableExtra<Result>(EXTRA_RESULT)?.addressOptionsResult
Expand All @@ -34,7 +38,8 @@ internal class AddressElementActivityContract :
data class Args internal constructor(
internal val publishableKey: String,
internal val config: AddressLauncher.Configuration?,
@InjectorKey internal val injectorKey: String = DUMMY_INJECTOR_KEY
@InjectorKey internal val injectorKey: String = DUMMY_INJECTOR_KEY,
@ColorInt internal val statusBarColor: Int? = null
) : ActivityStarter.Args {

companion object {
Expand Down
@@ -1,10 +1,17 @@
package com.stripe.android.paymentsheet.addresselement

import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.ColumnScope
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import kotlin.math.min

// https://gist.github.com/ademar111190/34d3de41308389a0d0d8

fun CharSequence.levenshtein(other: CharSequence): Int {
internal fun CharSequence.levenshtein(other: CharSequence): Int {
if (this == other) { return 0 }
if (this.isEmpty()) { return other.length }
if (other.isEmpty()) { return this.length }
Expand Down Expand Up @@ -47,3 +54,18 @@ internal fun AddressDetails.editDistance(otherAddress: AddressDetails?): Int {
editDistance += (address?.state ?: "").levenshtein(comparedAddress?.state ?: "")
return editDistance
}

@Composable
internal fun ScrollableColumn(
modifier: Modifier = Modifier,
content: @Composable ColumnScope.() -> Unit
) {
Box(
modifier = Modifier.verticalScroll(rememberScrollState())
) {
Column(
modifier = modifier,
content = content
)
}
}
Comment on lines +58 to +71
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar to what we’re using in Link. Let me know if you prefer a different approach.