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

Add in app banner message for a new device #5257

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Oct 10, 2023

  • Rework how notifications work and introduce a InAppNotificationController
  • Add New Device notification for when the user logs in, informing the user about the newly added device.

This change is Reviewable

@linear
Copy link

linear bot commented Oct 10, 2023

DROID-92 Add in-app banner message for a new device

Add an in-app banner message that the user need to close for it to disappear, when the user has closed the in-app banner message the device name and time left are shown in the header (see referenced card), Confluence and Zeplin links in Parent card

@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch from 1798ae1 to f929d6a Compare October 10, 2023 09:50
@Rawa Rawa self-assigned this Oct 10, 2023
@Rawa Rawa added the Android Issues related to Android label Oct 10, 2023
@Rawa Rawa changed the title WIP: Add in app banner message for a new device droid 92 WIP: Add in app banner message for a new device Oct 10, 2023
@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch 6 times, most recently from 0915059 to b2c5858 Compare October 12, 2023 07:13
@Rawa Rawa changed the title WIP: Add in app banner message for a new device Add in app banner message for a new device Oct 12, 2023
@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch 2 times, most recently from 3779ee3 to 8fa9266 Compare October 13, 2023 09:12
@Rawa Rawa requested a review from Pururun October 13, 2023 09:16
@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch 2 times, most recently from c6c0aa3 to eb00f07 Compare October 13, 2023 10:21
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 5 of 68 files at r1, 76 of 76 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)


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

) {
    val previous = rememberPrevious(current = notification, shouldUpdate = { _, _ -> true })
    // Fix for animating t hide

Nit: to hide


android/lib/resource/src/main/res/values/strings.xml line 161 at r3 (raw file):

    <string name="vpn_permission_error_notification_title">VPN permission error</string>
    <string name="vpn_permission_error_notification_message">Always-on VPN might be enabled for another app</string>
    <string name="new_device_notification_title">NEW DEVICE CREATED</string>

I'm not sure about this, do we want to set all caps in the code or have all caps in the strings?

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/AccountExpiryNotificationUseCase.kt line 31 at r3 (raw file):

    private fun AccountExpiry.isCloseToExpiring(): Boolean {
        val threeDaysFromNow = DateTime.now().plusDays(3)

This magic number should be a constant.

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: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Pururun)


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

Previously, Pururun (Jonatan Rhodin) wrote…

Nit: to hide

Done


android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/AccountExpiryNotificationUseCase.kt line 31 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This magic number should be a constant.

Yeah, I agree, this was from the previous implementation, I've moved it into constants now


android/lib/resource/src/main/res/values/strings.xml line 161 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I'm not sure about this, do we want to set all caps in the code or have all caps in the strings?

Not sure, but it was all caps in messages.po, I'm unsure if that is case sensitive? Maybe @albin-mullvad has an opinion?

@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch 9 times, most recently from 9f68ccb to fa25d66 Compare October 17, 2023 13:02
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 13 of 13 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Rawa)


-- commits line 9 at r4:
Should probably be merged into "Rework notifications"

Also should also update the change log.


android/lib/resource/src/main/res/values/strings.xml line 161 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Not sure, but it was all caps in messages.po, I'm unsure if that is case sensitive? Maybe @albin-mullvad has an opinion?

If it is the text we get from messages.po I think we should keep it.

@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch from fa25d66 to ff21f4b Compare October 17, 2023 13:41
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: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


-- commits line 9 at r4:

Previously, Pururun (Jonatan Rhodin) wrote…

Should probably be merged into "Rework notifications"

Also should also update the change log.

Added them to the respective commits now 👍

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: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


-- commits line 9 at r4:

Previously, Rawa (David Göransson) wrote…

Added them to the respective commits now 👍

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

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


android/lib/resource/src/main/res/values/strings.xml line 161 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

If it is the text we get from messages.po I think we should keep it.

I think we should avoid all caps in our strings (and template) since no other notification titles are in all-caps in our strings (template). Would be nice to change/align this with desktop.

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: :shipit: complete! all files reviewed, all discussions resolved


android/lib/resource/src/main/res/values/strings.xml line 161 at r3 (raw file):

Previously, albin-mullvad wrote…

I think we should avoid all caps in our strings (and template) since no other notification titles are in all-caps in our strings (template). Would be nice to change/align this with desktop.

Actually some of the notification titles are all caps:
UPDATE AVAILABLE
UNSUPPORTED VERSION

but yes some are not:
Account credit expires soon
Blocking internet

Spoke with Desktop, they make all the titles all caps in translation for title but also capitalize all the strings. They also mentioned that if there is a quirk for some languages it makes sense to capitalize them in strings only (but this isn't they case right now).

For our consistency I think it would be nice to have a new task refactor the notification names so they all end with _notification_title ( like desktop has msgctxt of in-app-notifications) and are written in all caps.

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch from ff21f4b to 33ed311 Compare October 20, 2023 06:32
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 5 of 5 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the add-in-app-banner-message-for-a-new-device-droid-92 branch from 33ed311 to c43b476 Compare October 23, 2023 09:28
@Pururun Pururun merged commit f58efc2 into main Oct 23, 2023
15 checks passed
@Pururun Pururun deleted the add-in-app-banner-message-for-a-new-device-droid-92 branch October 23, 2023 11:37
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.

3 participants