-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
Start migration to Compose Navigation #1211
base: main-ose
Are you sure you want to change the base?
Conversation
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
I think this can be a good first PR on the topic. |
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
@Composable
fun Example() {
val navController = LocalNavController.current
navController.navigate(ExampleRoute) |
Signed-off-by: Arnau Mora <[email protected]>
AccountsActivity
to MainActivity
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.
Can we move the NavController
to the navigation
package? Just so that there are no packages that only contain one class.
But didn't have an in-depth look yet. Please @sunkup also have a look first (we can talk tomorrow about it).
@ArnyminerZ maybe you can make the suggested change and request a new review from me? I will use the opportunity to get to know compose navigation. |
Signed-off-by: Arnau Mora <[email protected]>
# Conflicts: # app/src/main/kotlin/at/bitfire/davdroid/sync/account/AddressBookAuthenticatorService.kt # app/src/main/res/xml/sync_prefs.xml
Done |
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.
Looks really good!
For some reason I am sceptical about moving the AccountsDrawerHandler
injection to the model. I feel like it does not really belong there, but in favor of the new navigation paradigm I can't give a good reason not to do it ...
See the comment about broken reference, but yeah, otherwise it's really nice.
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.
Deleting the AccountsActivity
breaks a reference from jtxBoard: "Drawermenu > Synchronization > Go to collections".
For now, since we don't want to break external references we would need to keep an AccountsActivity
and have it redirect to the new MainActivity
. See #1172 (comment)
Also I think we could inject the Also, the idea is that the intro check occurs / the intro is potentially shown when the app UI starts. In the past we could only put it into the launcher activity ( In the future we could put it into the |
Purpose
As the first step of #1172, started by renaming
AccountsActivity
toMainActivity
, and start adding navigation functionality.Short description
AccountsActivity
and its usages toMainActivity
.AccountsRoute
from the intent data ofMainActivity
.LocalNavController
which holds a reference to the currently being used nav controller.AccountsDrawerHandler
from activity to model to decouple it completely from activities.Checklist