-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Migrate codebase to Navigation 3 #1902
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
base: main
Are you sure you want to change the base?
Conversation
Source code is still left in api module. impl module is empty at this point.
implementation(projects.feature.topic) | ||
implementation(projects.feature.search) | ||
implementation(projects.feature.settings) | ||
implementation(projects.feature.interests.api) |
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.
It's a shame we have to have both api
and impl
dependencies for every feature module. I understand why this is the case (api
contains the public facing routes and impl
contains the entryproviders), however, it would be good if we could avoid this somehow.
app/src/main/kotlin/com/google/samples/apps/nowinandroid/di/BackStackProvider.kt
Outdated
Show resolved
Hide resolved
object NiaAppNavigation { | ||
@Provides | ||
@Singleton | ||
fun provideBackStack(backStackFactory: BackStackFactory) : SnapshotStateList<Any> = |
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.
How about introducing a stronger type, say NiaRoute
or NiaNavKey
, for the back stack items? This would:
- Make navigation keys more obvious
- Protect against accidentally adding the wrong type to the back stack
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.
Added marker interface NiaBackStackKey
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.
I don't like the explicit association with BackStack
because the route is more than that. Would prefer NiaRoute
or NiaNavKey
(I think this is ok, even though it doesn't extend NavKey
)
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.
Nav3 doesn't really use the term route
- its more of a nav2 convention so I don't think NiaRoute
is a good idea.
the route is more than that
I think that used to be the case, butcontentKey
has since then replaced all usages ofkey
in NavEntryDecorators and scene keys. So at this pointkey
is really just a backStack key - its only used to map backstack items back to a NavEntry.
wdyt?
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.
Which on a side note, with introduction of contentKey
, i'm even not so sure about the official NavKey
interface anymore. Is it super clear that it should be used just for key
and not other keys? Maybe? Maybe not?
): EntryProviderBuilder<Any>.() -> @JvmSuppressWildcards Unit = { | ||
entry<SearchRouteNav3> { key -> | ||
SearchRoute( | ||
onBackClick = backStack::removeLast, |
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.
This is required because SearchScreen
displays a back arrow (do we need it?). Is it always the case that we just pop the last back stack item when it's clicked? Need to think about this more.
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.
I think it's needed. The search screen covers the entire screen so there's really no convenient way to dismiss/go back from search screen without it. Users could use system back, but then this would apply to all back arrows in general - I believe the added convenience from back arrow has its value.
Or in other words, is there any harm to having it? So far the back behavior of removeLost() seems natural to me as I play around with search. Unless you wanted to make search into a list/detail 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.
Right, ok we definitely need it then. My issue is that we only know that removeLast
is the right thing to do here because we know that SearchRoute
is only ever displayed in a single pane, which may not always be the case.
If we didn't know the number of panes being displayed, what should the back behavior be? Pop everything up to and including SearchRoute
? If so, we should do that (maybe add a backStack.popUpTo(route, inclusive = true)
method). Or maybe we should try to call NavDisplay.onBack
.
...n/com/google/samples/apps/nowinandroid/feature/search/impl/navigation/SearchEntryProvider.kt
Outdated
Show resolved
Hide resolved
feature/topic/api/src/main/res/drawable/feature_topic_api_ic_topic_placeholder.xml
Outdated
Show resolved
Hide resolved
...lin/com/google/samples/apps/nowinandroid/feature/topic/impl/navigation/TopicEntryProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/google/samples/apps/nowinandroid/navigation/NiaNavDisplay.kt
Show resolved
Hide resolved
bd250e5
to
9b9cfe5
Compare
Bump agp versions and add navigation 3 dependency
9b9cfe5
to
b85823d
Compare
onBackClick = backStack::removeLast, | ||
onInterestsClick = key.onInterestsClick, | ||
onTopicClick = backStack::navigateToInterests, | ||
) |
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.
There's a bug here on main branch - filed #1906
b85823d
to
ce87666
Compare
Bump agp versions and add navigation 3 dependency
[WIP] This PR migrates the codebase to using Jetpack Navigation 3.
More details to follow...