-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Change special folder success to loading and change navigation #7569
Conversation
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.
To be consistent with the other transient screens the "Next" button should only be shown when the user has to manually specify the folder mapping.
...unt/setup/src/main/kotlin/app/k9mail/feature/account/setup/navigation/AccountSetupNavHost.kt
Outdated
Show resolved
Hide resolved
a1e3413
to
6b5d1ca
Compare
@@ -129,23 +129,14 @@ class SpecialFoldersViewModel( | |||
|
|||
private fun navigateNext() { |
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.
By changing this to fun navigateNext(isManualSetup: Boolean)
we can pass in the appropriate value at the call sites and don't need State.isManualSetup
.
@@ -33,7 +33,7 @@ class SpecialFoldersScreenKtTest : ComposeTest() { | |||
assertThat(onNextCounter).isEqualTo(0) | |||
assertThat(onBackCounter).isEqualTo(0) | |||
|
|||
viewModel.effect(Effect.NavigateNext) | |||
viewModel.effect(Effect.NavigateNext(true)) |
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.
Please use a named argument here and in all the other Effect.NavigateNext
constructor calls in tests.
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.
You always have fresh ideas, why not write them down for everyone to participate or automate this, thanks.
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.
Some things are hard to write down in a formal rule. But it always comes down to "how easy is this code to read?".
Easy to read: enableProgressIndicator(true)
Somewhat verbose, but fine: enableProgressIndicator(enable = true)
Really verbose: enableProgressIndicator(enableProgressIndicator = true)
Leaves the reader wondering about the parameter: Effect.NavigateNext(true)
The reader has a better idea what the argument is about: Effect.NavigateNext(isManualSetup = true)
Don't allow navigating back to transient special folders screen
Fixes #7553