Support animating an overlay out after returning a result #1066
Conversation
| Box(modifier) { | ||
| content() | ||
| key(overlayHostData) { overlayHostData?.let { data -> data.overlay.Content(data::finish) } } | ||
| val transition = updateTransition(overlayHost.currentOverlayData, label = null) |
There was a problem hiding this comment.
I think we should keep the original code, and branch when overlay is AnimatedOverlay. Minimises the chances of the experimental API changes causing issues between different Compose versions.
| // If transitioning from null to not null, we want to use the targetState. And vice-versa, | ||
| // if transitioning from not null to null, we want to use the currentState. | ||
| val overlayData by remember { | ||
| derivedStateOf { transition.currentState ?: transition.targetState } |
There was a problem hiding this comment.
The derivedStateOf isn't doing much here. You can remove it and just inline the null coalesce. Also, swapping the content is making me think that an AnimatedContent might be a better fit here.
There was a problem hiding this comment.
Let me take another shot using AnimatedContent. I ran into some issues with it in my first draft, but it definitely makes the transition spec calculation nicer...
On the other hand, maybe AnimatedOverlay.AnimatedContent should just observe the Transition's state and animate itself entirely...
@Composable
public abstract fun Transition<EnterExitState>.AnimatedContent(navigator: OverlayNavigator<Result>)
I don't love how we have to change the enter/exit inputs with each state change in the current design.
| } | ||
| transition.AnimatedVisibility( | ||
| visible = { it != null }, | ||
| modifier = Modifier.fillMaxSize(), |
There was a problem hiding this comment.
Hmmm, this fillMaxSize() will affect the content layout too. I think it's safer to remove this and let the overlay content define what it wants to do.
| * [AnimatedContent] is executed with with [AnimatedVisibilityScope] so that child animations can be | ||
| * coordinated with the overlay's animations. | ||
| */ | ||
| public abstract class AnimatedOverlay<Result : Any>( |
There was a problem hiding this comment.
I think we should mark this as an experimental API. I can see this API evolving.
There was a problem hiding this comment.
Even though the library itself isn't stable yet anyway?
There was a problem hiding this comment.
Up to you here. I still think we can add signal to what APIs are more stable than others?
I don't mind either way though. If you'd rather keep experimental annotations out of here (understandably), all good with me.
There was a problem hiding this comment.
yeah I think we're fine without it for now, most of the library would probably fall under that annotation's umbrella if we introduced it heh
bryanstern
force-pushed
the
animated_overlays
branch
from
72da5de to
ba028ff
Compare
|
Updated this to use One open question is whether Screen_recording_20231222_220709.webm |
|
LGTM! |
|
Thanks! |
| final override fun Content(navigator: OverlayNavigator<Result>) { | ||
| AnimatedContent( | ||
| targetState = Unit, | ||
| transitionSpec = { EnterTransition.None togetherWith ExitTransition.None }, |
There was a problem hiding this comment.
I believe we should be using enterTransition and exitTransition here
Currently, if we want an exit animation for an overlay, we must wait for that animated to complete before returning the [Result]. This results in a lot of perceived lag if the [Result] should update the screen it was launched from. While this can be worked around with injecting callbacks or composition locals, those aren't ideal.
This updates
ContentWithOverlaysto animate anAnimatedOverlayin and out instead of just adding and removing it from composition. I'm open to ways to improve this.