remove destinationId and replace it with the class name of routes #46
Conversation
There was a problem hiding this comment.
NavigateToOnTopOfEvent, NavigateToOnTopOfRootEvent, NavigateBackAndThenToEvent feel to me as combination of other events and probably should not be in the library's API.
Although BackToRootEvent is similar to NavigateToRootEvent it can stay since it's a simpler alias to basically reset to current root.
What if instead we have a way to define a sequence of events or ensure that all events that sent will be processed (not only the last one sent) ?
What do you think?
| */ | ||
| @VisibleForTesting(otherwise = PACKAGE_PRIVATE) | ||
| public data class NavigateToOnTopOfRootEvent( | ||
| internal val route: NavRoute, |
There was a problem hiding this comment.
Shouldn't this be NavRoot?
There was a problem hiding this comment.
Yes, but I'll remove the extra method after #47 is merged (will update this pr then)
| * itself will also be popped of the back stack. Then navigates to the given [route]. | ||
| */ | ||
| @VisibleForTesting(otherwise = PACKAGE_PRIVATE) | ||
| public data class NavigateBackAndThenToEvent( | ||
| public data class NavigateToOnTopOfEvent( |
There was a problem hiding this comment.
Can't this event be implemented as two events, navigate back with popUpTo and inclusive, and then using regular NavigateTo some destination?
There was a problem hiding this comment.
Yes. The main reason that we have it as an extra event is that it is also a possibility with navcontroller (calling navigate with options that use popUpTo). It's a bit nicer because it doesn't rely on automatically merging multiple nav operations into one
| * will also be popped of the back stack. | ||
| */ | ||
| @VisibleForTesting(otherwise = PACKAGE_PRIVATE) | ||
| public data class BackToRootEvent( |
There was a problem hiding this comment.
Isn't it the same as NavigateToRootEvent to the current root of backstack?
|
As mentioned the duplication for root will go away with #47.
This is already possible. You can for example call navigateTo multiple times to build a synthetic back stack. So it should probably be fine to replace |
There was a problem hiding this comment.
This is already possible. You can for example call navigateTo multiple times to build a synthetic back stack. So it should probably be fine to replace navigatoToOnTopOf with just making 2 calls
I guess then we can gradually replace it in our codebase and then clean it up 👍
|
Merged with master and marked |
Instead of having
destinationIdin the public API we will create ids internally based on the hashCode of the fully qualified class name of a route. Most APIs already had everything we need there were 2 changes other than removing the id from the destinations:navigateTowith pop up to is nownavigateToOnTopOf, a call would benavigatoToOnTopOf<FooDirections>(BarDirections())not super happy with the name here because it could also be interpreted as go to foo and then to bar. Also if you passinclusive = truethen Foo would also be removed and bar would be on top of foo's parentnavigateBackwith pop up to is nownavigateBackTo, e.g.navigateBackTo<FooDirections>(), here it works better