-
Notifications
You must be signed in to change notification settings - Fork 672
[link] Dynamic bottom sheet height #10838
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
Conversation
Diffuse output:
APK
|
// Workaround a race condition where the route changes while the soft keyboard is | ||
// animating in/out. Imagine navigating from screen A to B, where both screens are | ||
// supposed to be equal height. If the soft keyboard closes immediately after the | ||
// screen transition starts, the target height of screen B, which is locked in at | ||
// the start of the animation, will be too short. | ||
sizeTransform = if (isImeAnimating) { |
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.
👀 Demo of the problem:
demo-isImeAnimating.mov
// Keep height fixed to reduce animations caused by IME toggling on both | ||
// this screen and Verification screen. | ||
MinScreenHeightBox { |
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.
👀 Demo of the problem:
demo-MinScreenHeightBox.mov
AnimatedContent( | ||
targetState = state.paymentDetailsList.isEmpty(), | ||
transitionSpec = { LinkScreenTransition }, | ||
) { isLoading -> | ||
if (isLoading) { | ||
LinkLoadingScreen(Modifier.testTag(WALLET_LOADER_TAG)) | ||
} else { |
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.
👀 AnimatedContent
smooths out the transition. Demo of the problem:
demo-WalletBody-AnimatedContent.mov
/** | ||
* Displays a loading spinner in the center of the screen. | ||
* Will match the last screen size using [ProvideLinkScreenSize] to minimize size changes. | ||
*/ | ||
@Composable | ||
internal fun LinkLoadingScreen(modifier: Modifier = Modifier) { |
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.
👀 Demo of the problem below. Note how it goes from full screen → short → medium height. It now goes from full → medium height.
demo-LocalLinkScreenSize.mov
@@ -419,8 +419,15 @@ public final class com/stripe/android/link/ui/ComposableSingletons$LinkButtonKt | |||
public final class com/stripe/android/link/ui/ComposableSingletons$LinkContentKt { |
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.
Tip: review with Hide whitespace enabled
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.
code looking good, thanks for the hard work! Will get to manual testing today.
@OptIn(ExperimentalLayoutApi::class) | ||
@Composable | ||
private fun LinkNavHost( |
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.
nit - maybe we want to move this to its own file?
@Composable | ||
internal fun ProvideLinkScreenSize(size: IntSize?, content: @Composable () -> Unit) { | ||
val dpSize = with(LocalDensity.current) { | ||
size?.let { DpSize(it.width.toDp(), it.height.toDp()) } | ||
} | ||
CompositionLocalProvider( | ||
LocalLinkScreenSize provides dpSize, | ||
content = content | ||
) | ||
} |
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 this is used outside of the Loading screen composable as well, can we move it (and related functions) to a Screen utilities file?
Also can we add some docs to these composables for future reference?
* A [Box] that enforces a minimum height relative to the screen height. | ||
*/ | ||
@Composable | ||
internal fun MinScreenHeightBox( |
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.
We can probably make this file more generic and include the composables above.
18eb930
to
5d47ad6
Compare
@carlosmuvi-stripe I rebased, fixed conflicts, and did some minor cleanup/refactoring per request. |
private val LocalLinkScreenSizeInternal = compositionLocalOf<DpSize?> { null } | ||
|
||
/** | ||
* Current screen size rendered in [LinkNavHost]. | ||
*/ | ||
internal val LocalLinkScreenSize: CompositionLocal<DpSize?> = LocalLinkScreenSizeInternal |
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.
Note here how LocalLinkScreenSize
cannot be provided except by LinkNavHost
.
Summary
Make the Link bottom sheet have dynamic height (adjusts to its content size).
Getting this to work well was difficult. In PaymentSheet, we're able to simply apply
animateContentSize()
to the top-level bottom sheet container and call it a day, but for Link in PS it leads to an awfully janky UI:demo-animateContentSize.mov
The complications with the Link UX are:
NavHost
to route (with transition animations) between screensModalBottomSheetLayout
sAll of the above lead to animations clashing with one another, for example:
Complicating things further, many animation APIs don't work right when used in a
ModalBottomSheetLayout
bottom sheet or outright crash in nestedModalBottomSheetLayout
s (see demo app):demo-bottom-sheet-animation-apis.mov
Ultimately, the changes in this PR include:
NavHost
transition properties for animating between Link screensAnimatedContent
for animating components within Link screensLinkLoadingScreen
)ModalBottomSheetLayout
with anElementsBottomSheetLayout
adjacent to the other oneAlso in this commit history are different approaches I tried that didn't pan out (example). I decided not to rebase them away for posterity.
Motivation
https://jira.corp.stripe.com/browse/LINK_MOBILE-190
Testing
Screenshots
Below are recordings of running through the example app.
dbs-demo1.mov
dbs-demo2.mov