Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

fix(settings): change elk to moz social in notifications settings [MOSOWEB-89] #84

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

jpezninjo
Copy link

@jpezninjo jpezninjo commented Nov 14, 2023

Goal

Replace "Elk" text from Push Notifications settings with "Mozilla.social" MOSOWEB-89

Image of the affected spot in local:
image

How to Test this

I did some stuff in components/notification/NotificationPreferences.client.vue and pages/settings/notifications/push-notifications.vue to force my way into the route 🤷

Something about pwaEnabled disallows this page in local

@jpezninjo jpezninjo requested a review from a team as a code owner November 14, 2023 21:14
@@ -524,10 +524,10 @@
"unsupported": "Your browser does not support push notifications.",
"warning": {
"enable_close": "Close",
"enable_description": "To receive notifications when Elk is not open, enable push notifications. You can control precisely what types of interactions generate push notifications via the \"@:settings.notifications.show_btn{'\"'} button above once enabled.",
"enable_description_desktop": "To receive notifications when Elk is not open, enable push notifications. You can control precisely what types of interactions generate push notifications in \"Settings > Notifications > Push notifications settings\" once enabled.",
"enable_description": "To receive notifications when Mozilla.social is not open, enable push notifications. You can control precisely what types of interactions generate push notifications via the \"@:settings.notifications.show_btn{'\"'} button above once enabled.",
Copy link
Author

@jpezninjo jpezninjo Nov 14, 2023

Choose a reason for hiding this comment

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

The 3 translation entries are for the 3 different versions of this UI, based on a prop called closeableHeader and a css check for xl

Copy link

@wtfluckey wtfluckey left a comment

Choose a reason for hiding this comment

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

Good stuff. Will you make these changes for the German translation too?

@jpezninjo
Copy link
Author

@wtfluckey good call. Pushed up

@@ -479,10 +479,10 @@
"unsupported": "Ihr Browser unterstützt keine Push-Benachrichtigungen.",
"warning": {
"enable_close": "Schließen",
"enable_description": "Um Benachrichtigungen zu erhalten, wenn Elk nicht geöffnet ist, aktiviere Push-Benachrichtigungen. \nDu kannst genau steuern, welche Arten von Interaktionen Push-Benachrichtigungen generieren, indem du die Schaltfläche \"@:settings.notifications.show_btn{'\"'} oben aktivierest, sobald sie aktiviert ist.",
"enable_description_desktop": "Um Benachrichtigungen zu erhalten, wenn Elk nicht geöffnet ist, aktiviere Push-Benachrichtigungen. \nDu kannst unter „Einstellungen > Benachrichtigungen > Einstellungen für Push-Benachrichtigungen“ genau steuern, welche Arten von Interaktionen Push-Benachrichtigungen generieren, sobald diese aktiviert sind.",
"enable_description": "Um Benachrichtigungen zu erhalten, wenn Mozilla.social nicht geöffnet ist, aktiviere Push-Benachrichtigungen. \nDu kannst genau steuern, welche Arten von Interaktionen Push-Benachrichtigungen generieren, indem du die Schaltfläche \"@:settings.notifications.show_btn{'\"'} oben aktivierest, sobald sie aktiviert ist.",
Copy link

@cadeyrn cadeyrn Nov 15, 2023

Choose a reason for hiding this comment

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

It was already wrong in the previous version, but since you're already on it: "aktivierest" should be "aktivierst". I am not sure if that's the best word, and it's also not what the English original says. Additionally, "aktivierst" would be repeated twice in quick succession. This doesn't sound good and even makes the sentence a bit confusing.

Suggestion for enable_description: "Um Benachrichtigungen zu erhalten, wenn Mozilla.social nicht geöffnet ist, aktiviere Push-Benachrichtigungen. \nÜber die Schaltfläche "@:settings.notifications.show_btn{'"'} oben kannst genau steuern, welche Arten von Interaktionen Push-Benachrichtigungen generieren, sobald diese aktiviert ist."

This would be closer to the English translation and less confusing.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @cadeyrn, thanks for the suggestion! I've put your changes onto a new branch and will ask our product if these fixes can come in. PR is here - #87

Copy link

@anthony-liddle anthony-liddle left a comment

Choose a reason for hiding this comment

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

I can't speak to the German, but the English looks good

@jpezninjo jpezninjo merged commit 02b9fbe into main Nov 15, 2023
3 checks passed
@jpezninjo jpezninjo deleted the fix/push-notifications-elk-language branch November 15, 2023 23:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants