-
Notifications
You must be signed in to change notification settings - Fork 74
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
Added shared element transition between contact row and chat screen #74
base: main
Are you sure you want to change the base?
Conversation
@@ -119,6 +128,8 @@ fun ChatScreen( | |||
onVideoClick: (uri: String) -> Unit, | |||
prefilledText: String? = null, | |||
viewModel: ChatViewModel = hiltViewModel(), | |||
sharedTransitionScope: SharedTransitionScope, | |||
animatedContentScope: AnimatedContentScope |
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.
You may want to consider using Composition locals here if the hierarchy is very deep and you find yourself passing these scopes around quite a bit. https://developer.android.com/develop/ui/compose/animation/shared-elements#understand-scopes
) | ||
Text( | ||
text = contact.name, | ||
modifier = Modifier.Companion.sharedBounds( |
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.
You can remove the Companion
bit here.
onVideoClick = {}, | ||
) | ||
SharedTransitionScope { | ||
AnimatedContent(targetState = 1) {_ -> |
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.
Rather use AnimatedVisibility(true) here instead of AnimatedContent for previews.
) | ||
} | ||
composable( | ||
route = "videoPlayer?uri={videoUri}", |
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.
You probably want to eventually switch this to also use type safe classes instead of strings.
modifier = Modifier.fillMaxSize(), | ||
) | ||
SharedTransitionLayout { | ||
AnimatedContent(targetState = 1) { _ -> |
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.
Is this a preview? Or where is this used?
If shared elements aren't required in these examples, you could consider creating a Modifier.trySharedElement()
that doesn't nessecarily need the scopes and just does a no-op when scopes are not present, and adds the modifiers when scopes are present. Then in screens where its not used, you wouldn't provide the scopes.
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.
Otherwise I'd change this to AnimatedVisibility(true)
for a bit of a cleaner description.
) | ||
.padding(16.dp) | ||
.sharedBounds( | ||
sharedTransitionScope.rememberSharedContentState(key = "Bounds${chat.chatWithLastMessage.id}"), |
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.
If this shared element is quite complex - you might want to consider introducing a class that represents the key - For example like this https://developer.android.com/develop/ui/compose/animation/shared-elements#unique-keys. It means it'll be strongly typed, easier to navigate the codebase and find the corresponding keys, and harder to override if you were to add another shared element on the same screen.
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.
Looking good! Left a few suggestions to improve some bits. Can you include a screen recording for the result?
Added shared element/shared bounds transition between the contact row and the chat screen.
This demonstrates two simultaneous shared bounds transitions with a shared element transition for the icon.
There's also a bugfix for a Hilt-related crash in bubbles.