-
Notifications
You must be signed in to change notification settings - Fork 352
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
Add enable/disable button to Split Tunnelling #5641
Add enable/disable button to Split Tunnelling #5641
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.
Reviewed 4 of 10 files at r1, all commit messages.
Reviewable status: 4 of 10 files reviewed, 4 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 163 at r1 (raw file):
content: @Composable (modifier: Modifier, lazyListState: LazyListState) -> Unit ) {
nit: remove this empty newline
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 168 at r1 (raw file):
val scrollBehavior = TopAppBarDefaults.exitUntilCollapsedScrollBehavior(appBarState, canScroll = { canScroll }) Scaffold(
nit: add empty line before Scaffold
call to improve readability
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SplitTunnelingUiState.kt
line 6 at r1 (raw file):
data class SplitTunnelingUiState( val checked: Boolean = false,
Can the name of this property be clarified a bit?
Code quote:
checked
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SplitTunnelingViewModel.kt
line 85 at r1 (raw file):
} fun enableSplitTunneling(show: Boolean) {
There's a mix of words used to represent the setting/toggle state, such as enable
, show
, checked
etc. Can that be clarified throughout this PR?
b3276bc
to
6c16373
Compare
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.
Reviewable status: 1 of 10 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 163 at r1 (raw file):
Previously, albin-mullvad wrote…
nit: remove this empty newline
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 168 at r1 (raw file):
Previously, albin-mullvad wrote…
nit: add empty line before
Scaffold
call to improve readability
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SplitTunnelingUiState.kt
line 6 at r1 (raw file):
Previously, albin-mullvad wrote…
Can the name of this property be clarified a bit?
Done
android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SplitTunnelingViewModel.kt
line 85 at r1 (raw file):
Previously, albin-mullvad wrote…
There's a mix of words used to represent the setting/toggle state, such as
enable
,show
,checked
etc. Can that be clarified throughout this PR?
Changed to use enable
everywhere.
This is on hold waiting for design review. |
6c16373
to
fc86c9c
Compare
This is now on hold waiting for a stable release of the new compose version. |
83bb611
to
8fd089a
Compare
Since we updated the compose material 3 library this is now open for review again 🎉 |
Put on hold again since the design is getting an update. |
8fd089a
to
2052561
Compare
Design update is done, this is open for review again. |
aa3ad12
to
ef9a1e3
Compare
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.
Reviewed 2 of 10 files at r1, 2 of 6 files at r3, 7 of 8 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 158 at r6 (raw file):
navigationIcon: @Composable () -> Unit = {}, actions: @Composable RowScope.() -> Unit = {}, switch: @Composable RowScope.() -> Unit = {},
Is this still needed when we moved out the switch? Can we remove?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/TopBar.kt
line 242 at r6 (raw file):
@OptIn(ExperimentalMaterial3Api::class) @Composable fun MullvadMediumTopBarWithSwitch(
Same here, is this still needed or can it be removed?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SplitTunnelingScreen.kt
line 151 at r6 (raw file):
} private fun LazyListScope.enabledToggle(enabled: Boolean, onShowSplitTunneling: (Boolean) -> Unit) {
onShowSplitTunneling
-> onEnableSplitTunneling
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SplitTunnelingScreen.kt
line 180 at r6 (raw file):
} private fun LazyListScope.appList(
Nice work splitting this up, much more clear now 👏
ef9a1e3
to
059aaf0
Compare
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.
Reviewable status: 9 of 13 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SplitTunnelingScreen.kt
line 151 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
onShowSplitTunneling
->onEnableSplitTunneling
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SplitTunnelingScreen.kt
line 180 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Nice work splitting this up, much more clear now 👏
:)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 158 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Is this still needed when we moved out the switch? Can we remove?
No longer needed I just forgot to remove. Removed.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/TopBar.kt
line 242 at r6 (raw file):
Previously, Rawa (David Göransson) wrote…
Same here, is this still needed or can it be removed?
Yeah no longer needed. Removed.
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.
Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)
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.
Reviewed 1 of 10 files at r1, 2 of 8 files at r4, 2 of 4 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved
9f9a38c
to
a6ad8c9
Compare
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.
Reviewed 1 of 1 files at r5, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Pururun)
-- commits
line 14 at r8:
nit: I suggest simplifying this message a bit to just Update translations
which we usually use
Code quote:
Update messages.pot with disable split tunneling
CHANGELOG.md
line 38 at r8 (raw file):
#### Android - Add support for all screen orientations. - Add enable split tunneling toggle.
nit: since it's not just for "enabling" I suggest changing to something like: Add toggle for enabling or disabling split tunneling.
Co-Authored-By: Boban Sijuk <[email protected]>
Co-Authored-By: Boban Sijuk <[email protected]>
6de5838
to
4a9f3b1
Compare
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.
Reviewable status: 12 of 14 files reviewed, all discussions resolved (waiting on @albin-mullvad and @Rawa)
Previously, albin-mullvad wrote…
nit: I suggest simplifying this message a bit to just
Update translations
which we usually use
Done
CHANGELOG.md
line 38 at r8 (raw file):
Previously, albin-mullvad wrote…
nit: since it's not just for "enabling" I suggest changing to something like:
Add toggle for enabling or disabling split tunneling.
Done
Preview:
To align with desktop.
This is continued from: #5554
This change is