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

Make scene provide an AnimatedContentScope to the composable content #312

Merged

Conversation

StylianosGakis
Copy link
Contributor

@StylianosGakis StylianosGakis commented Apr 17, 2024

This scope is the one that drives the navigation transition, which allows callers to use it to drive other animations inside the screen itself as it animates into or out of the NavHost.
Notably, this allows callers to use the shared element transitions.

My only one concern left here is that the ComposeRoute interface seems to have been public, even though callers probably had no reason to use the interface in a meaningful way, much less extend it so that this change would be a breaking API change for them. Am I missing something here? Should that change also happen in a backwards compatible way here? Please do let me know if it is the case and I can try and look further into that instead.

Shared more thoughts here #308 (comment)

""",
level = DeprecationLevel.WARNING,
)
internal fun sceneRouteWithoutAnimatedContent(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to do something like what androidx.navigation does here https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:navigation/navigation-compose/src/main/java/androidx/navigation/compose/NavGraphBuilder.kt;l=57-60?q=%22ComposeNavigator.Destination(provider%22 which calls this internal deprecated function here https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:navigation/navigation-compose/src/main/java/androidx/navigation/compose/ComposeNavigator.kt;l=106-110 but for them it worked because the API also changed in a meaningful way besides just adding the AnimatedContentScope.

In our case here if I add it as a secondary constructor then there's ambiguity between the two of them and it's impossible to differentiate between the two. So I opted for an internal deprecated function to exist solely for this purpose.

@Tlaster
Copy link
Owner

Tlaster commented Apr 18, 2024

The reason ComposeRoute has been made public is simply because I haven’t been diligent enough in tidying up my code. Indeed, it should be internal.
And by the way PreCompsoe uses ktlint with spotless to do the linting things, so make sure to run ./gradlew spotlessApply.

This scope is the one that drives the navigation transition, which
allows callers to use it to drive other animations inside the screen
itself as it animates into or out of the NavHost.
Notably, this allows callers to use the shared element transitions.
@StylianosGakis StylianosGakis force-pushed the stylianos/provide-animated-content-scope branch from 2c4d92d to e03b0f7 Compare April 18, 2024 09:03
@StylianosGakis
Copy link
Contributor Author

StylianosGakis commented Apr 18, 2024

Thanks for the heads up, I ran ./gradlew spotlessApply and it fixed the 2 places I forgot to add a trailing comma.
And since there's no problem with changing the signature of ComposeRoute then I would say this PR is ready for you to review if you want this change to happen this way or not. I am happy with it as it is 😊

@Tlaster
Copy link
Owner

Tlaster commented Apr 19, 2024

Seems like there's a build issue, other than that this PR LGTM!

@StylianosGakis
Copy link
Contributor Author

The error message was a bit cryptic, but I think my latest commit addresses it.
Let CI run whenever you are able to, and I am hoping it will pass this time

Copy link
Owner

@Tlaster Tlaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! LGTM!

@Tlaster Tlaster merged commit 8e6ca3a into Tlaster:master Apr 19, 2024
2 checks passed
@StylianosGakis StylianosGakis deleted the stylianos/provide-animated-content-scope branch April 19, 2024 10:09
@StylianosGakis
Copy link
Contributor Author

Now we wait for the shared elements APIs to drop in multiplatform, and for SeekableTransition to also update to the new API which those apis rely on.
I wrongly assumed it's CMP lacking behind here JetBrains/compose-multiplatform#4659 but turns out it's just that those are all in the alpha releases. So when CMP starts releasing alphas with 1.7 I will try to come back here and get a sample working again. Hopefully that time without runtime crashes of course 😅

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