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 cut-off payment methods carousel in payment sheet #5424
Conversation
a70aab9
to
de53098
Compare
de53098
to
03f61e8
Compare
@@ -42,10 +44,10 @@ internal fun PaymentMethodsUI( | |||
paymentMethods: List<SupportedPaymentMethod>, | |||
selectedIndex: Int, | |||
isEnabled: Boolean, | |||
onItemSelectedListener: (SupportedPaymentMethod) -> Unit | |||
onItemSelectedListener: (SupportedPaymentMethod) -> Unit, | |||
state: LazyListState = rememberLazyListState() |
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.
Pulling this out so that I can pass in a specific firstVisibleItemIndex
in tests.
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.
Won't this code below already scroll to have the selected item as the first visible?
LaunchedEffect(selectedIndex) {
state.scrollToItem(selectedIndex, 0)
}
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.
Yep, you’re right. I initially thought that the LaunchedEffect
didn’t work properly in the test, but turns out it was because I also passed a wrong selectedIndex
in my test at the same time.
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.
lgtm
@@ -42,10 +44,10 @@ internal fun PaymentMethodsUI( | |||
paymentMethods: List<SupportedPaymentMethod>, | |||
selectedIndex: Int, | |||
isEnabled: Boolean, | |||
onItemSelectedListener: (SupportedPaymentMethod) -> Unit | |||
onItemSelectedListener: (SupportedPaymentMethod) -> Unit, | |||
state: LazyListState = rememberLazyListState() |
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.
Won't this code below already scroll to have the selected item as the first visible?
LaunchedEffect(selectedIndex) {
state.scrollToItem(selectedIndex, 0)
}
paymentsheet/src/main/java/com/stripe/android/paymentsheet/PaymentMethodsUI.kt
Outdated
Show resolved
Hide resolved
03f61e8
to
991cef3
Compare
991cef3
to
d5b71c3
Compare
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.
Thanks!
Summary
This pull request switches to using
contentPadding
for the payment methods carousel, which fixes a UX issue where the list was cut off prematurely at the start when scrolling.Motivation
UX polish.
Testing
Screenshots
Changelog
Nothing to add.