Use full back stack to determine type of transition animation #2104
Conversation
|
Thanks for the contribution! Before we can merge this, we need @kvaster to sign the Salesforce Inc. Contributor License Agreement. |
|
It look like salesforce-cla bugged... I've signed the CLA, it answered with an error and now it is saying me that CLA is already signed... |
|
Ya the cla bot is bugged, I'll fix it here |
| } else if (targetState.screen == initialState.screen) { | ||
| // Target screen has not changed, probably we should not show any animation even if the back stack is changed | ||
| return@spec EnterTransition.None togetherWith ExitTransition.None | ||
| } else if (initialState.backStack.contains(targetState.screen)) { |
There was a problem hiding this comment.
The contains here might not work as expected if we navigate to a screen already in the backstack.
So we probably want to compare the full record using the full backstack (the args given to us in DefaultAnimatedState), and determine if the initial and target backstacks have diverged.
There was a problem hiding this comment.
I'm sorry for such late answer...
I was thinking about how exactly we can/should compare backstacks. And I really think that 'contains' covers really more situations. If target screen was already in stack - we're moving back. In other case screen is new - it's GoTo.
There was a problem hiding this comment.
Also record itself contains only Screen...
There was a problem hiding this comment.
Ah sorry, I wasn't clear with this:
if we navigate to a screen already in the back stack.
If we're navigating to a new ScreenA instance with goTo and an equals ScreenA already exists in the backstack.
listOf(ScreenC, ScreenB, ScreenA)
goTo(ScreenA)
listOf(ScreenA, ScreenC, ScreenB, ScreenA)Using Record in this case is useful because it'll contain a unique key per back stack entry.
There was a problem hiding this comment.
Thanks. I will update my PR.
1a22ade to
e2e7d7f
Compare
|
I've updated my PR. Now NavArgument is used to compare stacks. |
Awesome, looks good! Thanks for making the record changes 🚀 I'm cool with removing |
|
backStackDepth propogation removed. |
...dation/src/commonMain/kotlin/com/slack/circuit/foundation/animation/AnimatedNavDecoration.kt
Show resolved
Hide resolved
|
I'm sorry, forgot to run tests and to commit code formatting changes. Now it should be OK. |
cca27f4 to
d6fff08
Compare
|
I've made rebase and resolved conflicts with main branch. |
Currently type of transition animation is determined only by backstack depth (and by root screen change), but this doesn't cover complex backstack changes - i.e. when you pop several screen and add one more on top.
This PR always treat new screens as GoTo animation. Pop animation will be played only if we're back to screen which was already in a back stack.
Fixes #2075
Probably screem, rootScreen, backStackDepth should be removed from AnimatedNavState - we can get them from backStack property. Also backStackDepth may be removed from other functions.