Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DAGalpin
Copy link

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.

@ashnohe ashnohe requested review from ashnohe, trambui09 and riggaroo May 30, 2024 16:28
@@ -119,6 +128,8 @@ fun ChatScreen(
onVideoClick: (uri: String) -> Unit,
prefilledText: String? = null,
viewModel: ChatViewModel = hiltViewModel(),
sharedTransitionScope: SharedTransitionScope,
animatedContentScope: AnimatedContentScope
Copy link
Contributor

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(
Copy link
Contributor

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) {_ ->
Copy link
Contributor

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}",
Copy link
Contributor

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) { _ ->
Copy link
Contributor

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.

Copy link
Contributor

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}"),
Copy link
Contributor

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.

Copy link
Contributor

@riggaroo riggaroo left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants