-
Notifications
You must be signed in to change notification settings - Fork 355
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
Update changelog presentation #7394
base: main
Are you sure you want to change the base?
Conversation
8750c27
to
16442d7
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 20 of 20 files at r1, 1 of 1 files at r2.
Reviewable status: 19 of 21 files reviewed, 2 unresolved discussions (waiting on @kl)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/notificationbanner/NotificationData.kt
line 42 at r2 (raw file):
) : this( AnnotatedString(title), message?.let { NotificationMessage.Text(AnnotatedString(it)) },
This is just a general comment about clickable text, but any reason we did use an icon as for external links instead of the clickable text?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewChangelogNotificationUseCase.kt
line 18 at r2 (raw file):
operator fun invoke() = combine( userPreferencesRepository.preferencesFlow,
Do we need to consider ALWAYS\_SHOW\_CHANGELOG
or do we already do that in the repository? Maybe that property is not longer relevant?
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: 19 of 21 files reviewed, 2 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/notificationbanner/NotificationData.kt
line 42 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This is just a general comment about clickable text, but any reason we did use an icon as for external links instead of the clickable text?
We already have one clickable icon in that view (the X to close the notification), so maybe strange to have more?
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewChangelogNotificationUseCase.kt
line 18 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Do we need to consider
ALWAYS\_SHOW\_CHANGELOG
or do we already do that in the repository? Maybe that property is not longer relevant?
Good question!
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: 19 of 23 files reviewed, 2 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewChangelogNotificationUseCase.kt
line 18 at r2 (raw file):
Previously, kl (Kalle Lindström) wrote…
Good question!
Removed in latest commit
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 3 of 20 files at r1, 2 of 2 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kl and @Pururun)
android/lib/resource/src/main/res/values/strings.xml
line 179 at r5 (raw file):
</string> <string name="new_changelog_notification_title">NEW VERSION INSTALLED</string> <string name="new_changelog_notification_message">Click here to see what\'s new.</string>
See other comment regarding curly apostrophes
Code quote:
what\'s
android/lib/resource/src/main/res/values/strings.xml
line 392 at r5 (raw file):
<string name="share">Share…</string> <string name="app_info">App info</string> <string name="changelog_title">What\'s new</string>
I believe this string is incorrect since we should only use curly apostrophes (’
). We also shouldn't need the backslash.
For example see this previous issue: https://linear.app/mullvad/issue/DROID-1290
Code quote:
What\'s new
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 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/notificationbanner/NotificationData.kt
line 42 at r2 (raw file):
Previously, kl (Kalle Lindström) wrote…
We already have one clickable icon in that view (the X to close the notification), so maybe strange to have more?
Right I did not considered the close button, in that case it is all good!
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewChangelogNotificationUseCase.kt
line 11 at r5 (raw file):
import net.mullvad.mullvadvpn.repository.UserPreferencesRepository class NewChangelogNotificationUseCase(
This should considered if we actually have anything in the changelog. Should probably not notify if we have an empty changelog.
6c095d7
to
e66061b
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: 18 of 25 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad, @kl, and @Pururun)
android/lib/resource/src/main/res/values/strings.xml
line 179 at r5 (raw file):
Previously, albin-mullvad wrote…
See other comment regarding curly apostrophes
Fixed, but we need to notify desktop team since they use the wrong apostrophes on these strings.
android/lib/resource/src/main/res/values/strings.xml
line 392 at r5 (raw file):
Previously, albin-mullvad wrote…
I believe this string is incorrect since we should only use curly apostrophes (
’
). We also shouldn't need the backslash.For example see this previous issue: https://linear.app/mullvad/issue/DROID-1290
Fixed, but we need to notify desktop team since they use the wrong apostrophes on these strings.
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: 18 of 25 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewChangelogNotificationUseCase.kt
line 11 at r5 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This should considered if we actually have anything in the changelog. Should probably not notify if we have an empty changelog.
Fixed
This change is