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 title overflow and replace switch #5310

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Oct 17, 2023

  • Replaces the custom switch with the material switch
  • Fixes text overflow

This change is Reviewable

@linear
Copy link

linear bot commented Oct 17, 2023

DROID-385 Custom DNS toggle looks strange in 2023.6

david said:

Custom DNS toggle looks strange in 2023.6

We should probably replace the switch with the material3 one.

@Rawa Rawa self-assigned this Oct 17, 2023
@Rawa Rawa added the Android Issues related to Android label Oct 17, 2023
@Rawa Rawa requested review from sabercodic and Pururun October 17, 2023 14:17
@Rawa Rawa changed the title Fix headertitle overflow and replace switch Fix title overflow and replace switch Oct 17, 2023
@Rawa
Copy link
Contributor Author

Rawa commented Oct 17, 2023

I've posted the question to design whether we should keep the toggles closer to material or our desktop design.

Update: Design wanted it as per Zeplin, I've fixed and updated it.

@Rawa Rawa force-pushed the custom-dns-toggle-looks-strange-in-20236-droid-385 branch 2 times, most recently from 5c5fd71 to f625ae7 Compare October 17, 2023 14:40
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 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Switch.kt line 49 at r1 (raw file):

    thumbContent: (@Composable () -> Unit)? = {
        // This is needed to ensure the thumb always is big in off mode
        Spacer(modifier = Modifier.size(24.dp))

Probably should not be a magic number.
Is there any way to get this number from the material library?
Otherwise I guess we should set in Dimens.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Switch.kt line 69 at r1 (raw file):

fun mullvadSwitchColors(): SwitchColors =
    SwitchDefaults.colors(
        checkedThumbColor = MaterialTheme.colorScheme.surface, // TODO Change

Since we have now merged the button PR, we can use selected() here instead and thus we can change that color in the future.

@Rawa Rawa force-pushed the custom-dns-toggle-looks-strange-in-20236-droid-385 branch from f625ae7 to 0a14773 Compare October 18, 2023 09:34
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: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Switch.kt line 49 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Probably should not be a magic number.
Is there any way to get this number from the material library?
Otherwise I guess we should set in Dimens.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Switch.kt line 69 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Since we have now merged the button PR, we can use selected() here instead and thus we can change that color in the future.

Done.

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 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the custom-dns-toggle-looks-strange-in-20236-droid-385 branch from 0a14773 to ac9358c Compare October 18, 2023 10:58
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 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the custom-dns-toggle-looks-strange-in-20236-droid-385 branch from ac9358c to b8f3314 Compare October 18, 2023 18:10
@Pururun Pururun force-pushed the custom-dns-toggle-looks-strange-in-20236-droid-385 branch from b8f3314 to 453992d Compare October 18, 2023 18:47
@Pururun Pururun merged commit fe0b309 into main Oct 18, 2023
12 checks passed
@Pururun Pururun deleted the custom-dns-toggle-looks-strange-in-20236-droid-385 branch October 18, 2023 19:49
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.

2 participants