Topic 6/ Frontend Add alerts related translations #28

imenattatra · 2024-12-13T12:41:12Z

Related to the translation project : https://github.com/metr-systems/backlog/issues/3476

What type of PR is this?

This is related to redash translation project and covers texts related to Alerts.

How is this tested?

Unit tests (pytest, jest)
Manually

This is currently deployed to staging

Follow-up:

Translation related to vis-lib

helenalebreton · 2025-01-07T12:59:25Z

@imenattatra my comments

the time parameter is not yet translated

small think here as well when trying to add a destination and no choice is available

do you know if there is anything we can do about these ?

if not - please do not consider as a priority. At this time it is only a question, and does not need to be addressed in terms of implementation.

berinhard

Thanks a lot @imenattatra for pushing this through. This i18next was a good choice from the API I'm seeing here. I added some comments to better understand it, but I feel we should stick with one strategy on how to do translation and stick with it.

Question: would it be possible to define this t = useTranslation("Alerts") in a single place that can be used by every component? Like a type of global variable?

berinhard · 2025-01-07T13:39:33Z

client/app/pages/alert/Alert.jsx

@@ -113,12 +117,12 @@ class Alert extends React.Component {

    return AlertService.save(alert)
      .then(alert => {
-        notification.success("Saved.");
+        notification.success(i18next.t("Saved."));


Should we have the Alerts: prefix here too?

Suggested change

notification.success(i18next.t("Saved."));

notification.success(i18next.t("Alerts:Saved."));

because "Saved." is used in many places across the app, i did put it under the common namespace which is set as default, so we don't need to write common:

berinhard · 2025-01-07T13:40:02Z

client/app/pages/alert/AlertEdit.jsx

@@ -56,32 +58,32 @@ export default class AlertEdit extends React.Component {
          <DynamicComponent name="AlertEdit.HeaderExtra" alert={alert} />
          <Button className="m-r-5" onClick={() => this.cancel()}>
            <i className="fa fa-times m-r-5" aria-hidden="true" />
-            Cancel
+            {i18next.t("Cancel")}


Suggested change

{i18next.t("Cancel")}

{i18next.t("Alerts:Cancel")}

same for Cancel as for Saved., it is used in many places across the app, i did put it under the common namespace which is set as default, so we don't need to write common:

Ahhhhhh ok, got it! Thanks for the explanation 🙏

berinhard · 2025-01-07T13:40:12Z

client/app/pages/alert/AlertEdit.jsx

          </Button>
          <Button type="primary" onClick={() => this.save()}>
            {saving ? (
              <span role="status" aria-live="polite" aria-relevant="additions removals">
                <i className="fa fa-spinner fa-pulse m-r-5" aria-hidden="true" />
-                <span className="sr-only">Saving...</span>
+                <span className="sr-only">{i18next.t("Saving...")}</span>


Suggested change

{i18next.t("Saving...")}

{i18next.t("Alerts:Saving")}...

I will remove the three dots and other characters from the translated text in a separate PR since these Saving.. and (Help) for example are used across the app at many places... this is also why you find no namespace prefix to them since they are in the common default namespace

berinhard · 2025-01-07T13:42:18Z

client/app/pages/alert/AlertEdit.jsx

-                Setup Instructions <i className="fa fa-question-circle" aria-hidden="true" />
-                <span className="sr-only">(help)</span>
+                {i18next.t("Alerts:Setup Instructions")} <i className="fa fa-question-circle" aria-hidden="true" />
+                <span className="sr-only">{i18next.t("(help)")}</span>


It's nice to leave non-translatable characters like ( or ... outside of the translation string. This will make it simpler for people to add translation since they won't be worry about the context where such texts are being used.

Suggested change

{i18next.t("(help)")}

({i18next.t("Alerts:help")})

yes, (same reply as previous one :D ) I will remove the three dots and other characters from the translated text in a separate PR since these Saving.. and (Help) for example are used across the app at many places... this is also why you find no namespace prefix to them since they are in the common default namespace

berinhard · 2025-01-07T13:43:12Z

client/app/pages/alert/AlertNew.jsx

@@ -79,16 +81,16 @@ export default class AlertNew extends React.Component {
                  {saving && (
                    <span role="status" aria-live="polite" aria-relevant="additions removals">
                      <i className="fa fa-spinner fa-pulse m-r-5" aria-hidden="true" />
-                      <span className="sr-only">Saving...</span>
+                      <span className="sr-only">{i18next.t("Saving...")}</span>


Suggested change

{i18next.t("Saving...")}

{i18next.t("Alerts:Saving")}...

same as previous one also, thanks

berinhard · 2025-01-07T13:47:56Z

client/app/pages/alert/components/AlertDestinations.jsx

          <Button
            data-test="ShowAddAlertSubDialog"
            type="primary"
            size="small"
            className="add-button"
            onClick={this.showAddAlertSubDialog}>
-            <i className="fa fa-plus f-12 m-r-5" aria-hidden="true" /> Add
+            <i className="fa fa-plus f-12 m-r-5" aria-hidden="true" /> {i18next.t("Add")}


Suggested change

 {i18next.t("Add")}

 {i18next.t("Alerts:Add")}

same for Add

berinhard · 2025-01-07T13:48:37Z

client/app/pages/alert/components/MenuButton.jsx

@@ -12,6 +14,7 @@ import EllipsisOutlinedIcon from "@ant-design/icons/EllipsisOutlined";
 import PlainButton from "@/components/PlainButton";

 export default function MenuButton({ doDelete, canEdit, mute, unmute, muted }) {
+  const { t } = useTranslation();


Suggested change

const { t } = useTranslation();

const { t } = useTranslation("Alerts");

Here i found myself between deciding either to use const { t } = useTranslation("Alerts"); but then i will need to find a solution for many expressions that fall in the case of common namespace or just not specify the prefix earlier and specify it only when needed... I took the second decision

berinhard · 2025-01-07T13:49:17Z

client/app/pages/alert/components/MenuButton.jsx

+      title: t("Alerts:Delete Alert"),
+      content: t("Alerts:Are you sure you want to delete this alert?"),


Since we've defined the t object as the translation form alerts, I feel we don't need this prefix here. Is it correct?

Suggested change

title: t("Alerts:Delete Alert"),

content: t("Alerts:Are you sure you want to delete this alert?"),

title: t("Delete Alert"),

content: t("Are you sure you want to delete this alert?"),

This is related to the previous comment

berinhard · 2025-01-07T13:49:24Z

client/app/pages/alert/components/MenuButton.jsx

+              <PlainButton onClick={() => execute(unmute)}>{t("Alerts:Unmute Notifications")}</PlainButton>
            ) : (
-              <PlainButton onClick={() => execute(mute)}>Mute Notifications</PlainButton>
+              <PlainButton onClick={() => execute(mute)}>{t("Alerts:Mute Notifications")}</PlainButton>


berinhard · 2025-01-07T13:53:53Z

client/app/pages/alert/components/Rearm.jsx

+          <Trans i18nKey="Alerts:just_once">
+            Just once <em>until back to normal</em>
+          </Trans>


I see this as a 3rd approach on defining translations. I personally would stick with one of the previous one. If I understood it correctly, this Trans block requires this id which will be used to find the translated content. This can be harder for non-developer people to add translation because it requires they to navigate through the code to find the matching i18nKey and add the translated content.

Is this different because, within the translated context, we have this em tags?

I used Trans because it is designed for jsx code translation , so definitely it works as expected here. Also useTranslation or simple i18next.t does not work as expected with these jsx especially when it has nested content such as until back to normal

non developer people can check the english file, since it will include the word per word translation .. we don't have much of these cases anyway

imenattatra · 2025-01-09T10:21:39Z

@imenattatra my comments the time parameter is not yet translated

small think here as well when trying to add a destination and no choice is available

do you know if there is anything we can do about these ?

if not - please do not consider as a priority. At this time it is only a question, and does not need to be addressed in terms of implementation.

I corrected the two cases above : the time parameter and the alert destination case, but for the status, i don't want to touch it now because they are setting it from the backend but then i found out places where they check against the values, so if i touch one place and not everywhere something my stop working as it should , i prefer to do it separately later.

imenattatra · 2025-01-14T13:50:04Z

@helenalebreton and @berinhard I created this issue so that i don't forget about alerts status and the characters in translation : https://github.com/metr-systems/backlog/issues/3722, so that I can merge this PR :D , what do you think about it?

helenalebreton · 2025-01-14T13:59:05Z

@helenalebreton and @berinhard I created this issue so that i don't forget about alerts status and the characters in translation : metr-systems/backlog#3722, so that I can merge this PR :D , what do you think about it?

@imenattatra please consider it no priority for now. We will take a look at it only later after we looked into other UI isseus (not linked to translation) from redash

berinhard · 2025-01-14T16:01:35Z

Nice strategy here @imenattatra. I'm good with merging this PR if pending improvements are mapped to other issues.

extract alert related text to translate

a163d88

imenattatra changed the title ~~Add alerts translation~~ Topic 6/ Frontend Add alerts related translations Dec 13, 2024

imenattatra changed the base branch from metr-main to epic/ui-translation December 13, 2024 12:42

update translation for alerts related texts

24b0480

imenattatra force-pushed the add-alerts-translation branch from c542853 to 24b0480 Compare December 13, 2024 16:50

imenattatra added 2 commits January 6, 2025 17:30

update Alerts translation related to 'Just once'

d55dbf4

update Alerts 'Last Updated At'

346ad1a

imenattatra marked this pull request as ready for review January 7, 2025 12:45

imenattatra requested review from helenalebreton and a team January 7, 2025 12:45

imenattatra self-assigned this Jan 7, 2025

berinhard reviewed Jan 7, 2025

View reviewed changes

imenattatra added 3 commits January 7, 2025 17:04

update alert Rearm related translation

56152ae

finish translation of the time paramter for alerts

e77ce1d

add translation for Select Items Diaglog

217df1f

imenattatra requested a review from berinhard January 9, 2025 11:18

imenattatra merged commit 9dab6f2 into epic/ui-translation Jan 14, 2025

imenattatra deleted the add-alerts-translation branch January 14, 2025 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topic 6/ Frontend Add alerts related translations #28

Topic 6/ Frontend Add alerts related translations #28

imenattatra commented Dec 13, 2024 •

edited

Loading

helenalebreton commented Jan 7, 2025 •

edited

Loading

berinhard left a comment

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025 •

edited

Loading

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

berinhard Jan 7, 2025

imenattatra Jan 9, 2025

imenattatra Jan 9, 2025

imenattatra commented Jan 9, 2025

imenattatra commented Jan 14, 2025

helenalebreton commented Jan 14, 2025

berinhard commented Jan 14, 2025

	notification.success(i18next.t("Saved."));
	notification.success(i18next.t("Alerts:Saved."));

	<span className="sr-only">{i18next.t("Saving...")}</span>
	<span className="sr-only">{i18next.t("Alerts:Saving")}...</span>

	<span className="sr-only">{i18next.t("(help)")}</span>
	<span className="sr-only">({i18next.t("Alerts:help")})</span>

	<i className="fa fa-plus f-12 m-r-5" aria-hidden="true" /> {i18next.t("Add")}
	<i className="fa fa-plus f-12 m-r-5" aria-hidden="true" /> {i18next.t("Alerts:Add")}

	const { t } = useTranslation();
	const { t } = useTranslation("Alerts");

		title: t("Alerts:Delete Alert"),
		content: t("Alerts:Are you sure you want to delete this alert?"),

Topic 6/ Frontend Add alerts related translations #28

Topic 6/ Frontend Add alerts related translations #28

Conversation

imenattatra commented Dec 13, 2024 • edited Loading

What type of PR is this?

How is this tested?

Follow-up:

helenalebreton commented Jan 7, 2025 • edited Loading

berinhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imenattatra Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imenattatra commented Jan 9, 2025

imenattatra commented Jan 14, 2025

helenalebreton commented Jan 14, 2025

berinhard commented Jan 14, 2025

imenattatra commented Dec 13, 2024 •

edited

Loading

helenalebreton commented Jan 7, 2025 •

edited

Loading

imenattatra Jan 9, 2025 •

edited

Loading