Skip to content
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

Fix dialogs padding and spacing #5129

Merged
merged 5 commits into from
Sep 11, 2023

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Sep 8, 2023

Fixes inconsistencies between different dialogs and aligns it closer to the current design.


This change is Reviewable

@Rawa Rawa self-assigned this Sep 8, 2023
@linear
Copy link

linear bot commented Sep 8, 2023

DROID-314 fix settings dialogs' padding and spacing

Several of the dialogs have the wrong spacing compared to the design

@albin-mullvad albin-mullvad added the Android Issues related to Android label Sep 8, 2023
@Rawa Rawa force-pushed the fix-settings-dialogs-padding-and-spacing-droid-314 branch from 67325f2 to 854f0c2 Compare September 8, 2023 07:13
Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 13 files at r1, 3 of 4 files at r3, all commit messages.
Reviewable status: 12 of 13 files reviewed, 2 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/List.kt line 152 at r3 (raw file):

@Composable
fun ChangeListItem(text: String) {
    val smallPadding = Dimens.smallPadding

If there is just one usage for this just use it directly instead of define variable


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/QuantumResistanceInfoDialog.kt line 10 at r3 (raw file):

@Preview
@Composable
fun PreviewQuantumResistanceInfoDialog() {

make previews scope private. it applies for all previews

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ChangelogDialog.kt line 45 at r3 (raw file):

                modifier = Modifier.fillMaxWidth(),
                verticalArrangement =
                    Arrangement.spacedBy(dimensionResource(id = R.dimen.small_padding))

Should use Dimens.smallPadding instead.

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 13 files reviewed, 3 unresolved discussions (waiting on @Pururun and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/List.kt line 152 at r3 (raw file):

Previously, sabercodic wrote…

If there is just one usage for this just use it directly instead of define variable

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ChangelogDialog.kt line 45 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should use Dimens.smallPadding instead.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/QuantumResistanceInfoDialog.kt line 10 at r3 (raw file):

Previously, sabercodic wrote…

make previews scope private. it applies for all previews

Good catch! 👍

@Rawa Rawa force-pushed the fix-settings-dialogs-padding-and-spacing-droid-314 branch from 31de7c4 to d1e52cd Compare September 8, 2023 13:23
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 11 of 11 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 12 of 13 files at r1, 2 of 4 files at r3, 10 of 11 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)

Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

@Rawa Rawa force-pushed the fix-settings-dialogs-padding-and-spacing-droid-314 branch from 2ffb066 to 9d545de Compare September 11, 2023 09:03
@Pururun Pururun force-pushed the fix-settings-dialogs-padding-and-spacing-droid-314 branch from 9d545de to ecc2326 Compare September 11, 2023 10:04
@Pururun Pururun merged commit 4d28de5 into main Sep 11, 2023
9 checks passed
@Pururun Pururun deleted the fix-settings-dialogs-padding-and-spacing-droid-314 branch September 11, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants