Fix several UX issues in address element #5462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tillh-stripe
force-pushed
the
tillh/address-element-ui-fixes
branch
2 times, most recently
from
171398c
to
05263ae
Compare
|
Diffuse output: APKDEX |
tillh-stripe
force-pushed
the
tillh/address-element-ui-fixes
branch
from
05263ae
to
401309c
Compare
- Make sure there's an expand animation when opening the sheet - Make screens scrollable, which is necessary in landscape mode - Prevent search field from re-requesting focus after user hits enter - Adjust `Enter manually` font size - Prevent address sheet dismissal (only in auto-complete screen) and half-expanded state - Fix dismissal animation by overriding pending transitions
tillh-stripe
commented
Aug 25, 2022
| // this is a transparent Activity so we want to disable animations | ||
| overridePendingTransition(0, 0) | ||
| private fun setFadeAnimations() { | ||
| overridePendingTransition(AnimationConstants.FADE_IN, AnimationConstants.FADE_OUT) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Does it change anything? This activity has no UI
There was a problem hiding this comment.
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.
tillh-stripe
force-pushed
the
tillh/address-element-ui-fixes
branch
from
401309c
to
8e18fb3
Compare
tillh-stripe
commented
Aug 25, 2022
Comment on lines
+73
to
+76
| confirmStateChange = { | ||
| val route = navController.currentDestination?.route | ||
| route != AddressElementScreen.Autocomplete.route | ||
| } |
There was a problem hiding this comment.
I find that the user shouldn’t be able to exit from the sheet while in the autocomplete screen. Any concerns around this?
tillh-stripe
commented
Aug 25, 2022
Comment on lines
-858
to
-864
| 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; | ||
| } |
There was a problem hiding this comment.
These two things were made public by accident, right?
tillh-stripe
commented
Aug 25, 2022
Comment on lines
+58
to
+71
| @Composable | ||
| internal fun ScrollableColumn( | ||
| modifier: Modifier = Modifier, | ||
| content: @Composable ColumnScope.() -> Unit | ||
| ) { | ||
| Box( | ||
| modifier = Modifier.verticalScroll(rememberScrollState()) | ||
| ) { | ||
| Column( | ||
| modifier = modifier, | ||
| content = content | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to what we’re using in Link. Let me know if you prefer a different approach.
tillh-stripe
commented
Aug 25, 2022
Comment on lines
+161
to
+164
| .padding( | ||
| vertical = 8.dp, | ||
| horizontal = 16.dp | ||
| ) |
There was a problem hiding this comment.
Throughout this file, I made a few changes to where padding is applied. I did this to make sure that the click ripple effect looks correct, meaning that it spans the entire width and height of a row.
tillh-stripe
requested review from
brnunes-stripe,
michelleb-stripe,
jameswoo-stripe and
ccen-stripe
as code owners
jameswoo-stripe
approved these changes
Aug 25, 2022
brnunes-stripe
approved these changes
Aug 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request fixes a few UX issues in the address element screens:
ScrollableColumncomposable.Enter manuallyfont size.Motivation
Things that came up in the Address Element bug bash.
Testing
Screen recordings
Before
before.mp4
After
after.mp4
Changelog
Nothing to add.