Fix translation in repo selector #1295

jerabekjiri · 2021-11-24T16:04:09Z

Strings that are marked for translation don't seem to be working if imported from a different file. This is fixed by returning the string in the function and then evoking the function in component. Not sure this is the right way, I couldn't find any mention about this in the Lingui documentation, wdyt @himdel ?

  static REPOSITORYNAMES = {
    published: () => t`Published`,
    'rh-certified': () => t`Red Hat Certified`,
    community: () => t`Community`,
  };

before:

after:

jerabekjiri · 2021-11-24T16:15:11Z

This is the same case happening in Task Management, the tooltip should be translated, but it is imported from constants.tsx and therefore not being translated at the moment.

ansible-hub-ui/src/constants.tsx

Lines 278 to 279 in 2acbd97

    
           static TASK_NAMES = { 
        
             'galaxy_ng.app.tasks.promotion._remove_content_from_repository': t`Remove content from repository`,

himdel · 2021-11-24T17:11:22Z

Aah @jerabekjiri this is not really about different files, this is about the order in which things run..

Constants in modules are evaluated right away, before lingui switches the language to the right one .. so thay stay in English.

Normally (traditional gettext way), this is solved by a 2 step translation... using N_('Foo') instead of _('Foo') causes the string to get indexed into the translation catalog but returns the original string untranslated... so you'd have REPOSITORYNAMES = { published: N_("Published") }; and then use _(REPOSITORYNAMES.published) - we know the string is in the catalog thanks to the N_ so we can safely pass it to _ inside a variable.

Except lingui doesn't have an N_ equivalent that I can see - i18n._(str) can be used for the second part, but I see no way to index the strings without translating them right away, without doing something weird like { published: (t`My message`, `My message`), }.

So.. I think the approach of using functions instead of constants is valid, definitely for this case, but I'm less inclined to wrap every single constant with a human name in a function, that doesn't sound right.

So I think we should not reinvent the wheel, and come up with a way to achieve N_ with lingui.

Do you want to look into that or should I?

himdel · 2021-11-24T17:22:29Z

One more note.. Looking at gettext docs, N_ is also called gettext_noop... and looking for that + lingui led me to lingui/js-lingui#60 which adds a i18nMark to an ancient lingui ... aand it's no longer there :(.

Maybe we can reintroduce it? (Or, failing that, we could write our own babel N_ macro that translates N`foo` to t`foo`, `foo`, so they get indexed but return the original.)

jerabekjiri · 2021-12-03T12:26:12Z

We could theoretically use defineMessage. From docs: defineMessage is transformed into message descriptor. In other words, the message isn’t translated directly and can be used anytime later. That would mean we would still have to mark all constants community: defineMessage({message: 'Community' }). And then still use i18n._(community.id) to translate it.

himdel · 2021-12-08T00:46:02Z

Aah, sounds like you've found it, yeah, that sounds about right 👍 (docs - lazy translations, defineMessage)

jerabekjiri · 2021-12-10T14:05:05Z

Reworked it to the defineMessage. We still have to do i18n._(repo) in the component though.

Also found out this t({ message: 'Published' }) works as well.

src/constants.tsx

himdel · 2021-12-13T19:56:29Z

src/containers/task-management/task-list-view.tsx

@@ -271,7 +273,7 @@ export class TaskListView extends React.Component<RouteComponentProps, IState> {
      <tr aria-labelledby={name} key={index}>
        <td>
          <Link to={formatPath(Paths.taskDetail, { task: taskId })}>
-            <Tooltip content={Constants.TASK_NAMES[name] || name}>
+            <Tooltip content={i18n._(Constants.TASK_NAMES[name] || name)}>


Suggested change

<Tooltip content={i18n._(Constants.TASK_NAMES[name] || name)}>

<Tooltip content={i18n._(Constants.TASK_NAMES[name]) || name}>

we shouldn't expect the translation to exist when not in TASK_NAMES .. and lingui doesn't fall back to the original string in production

@himdel This will crash if Constants.TASK_NAMES[name] is undefined, i18n._(...) will throw error.

Constants.TASK_NAMES[name] ? (i18n._(Constants.TASK_NAMES[name]) || name) : name something like this would work. We first check if Constants.TASK_NAME[name] exists and then try to translate with i18n._(...), if not, return name. The code seems a bit longer though, that's why I used that first variant.

himdel · 2021-12-13T19:56:59Z

src/containers/task-management/task_detail.tsx

+                            {i18n._(
+                              Constants.TASK_NAMES[parentTask.name] ||
+                                parentTask.name,
+                            )}


Suggested change

{i18n._(

Constants.TASK_NAMES[parentTask.name] ||

parentTask.name,

)}

{i18n._(

Constants.TASK_NAMES[parentTask.name]) ||

parentTask.name

}

(+- prettier anyway :))

src/containers/task-management/task_detail.tsx

himdel · 2021-12-13T20:00:51Z

Also found out this t({ message: 'Published' }) works as well.

Works but it tries to translate the message when a language is set. Though, I suspect that means t(Constants.TASK_NAMES[childTask.name]) would work as well. (But it makes sense to stil use i18n._ to keep them distinct.)

Verified that defineMessage still causes the string to end up in the catalog with npm run gettext:extract 👍

himdel · 2021-12-13T20:04:42Z

Works in the UI, I think the repo selector changes make sense as well 👍

So, just that one defineMessage, and i18n._(x || y) -> i18n._(x) || y and it's ready :)

jerabekjiri · 2021-12-15T13:55:47Z

@himdel As I mentioned here comment, we can't do i18n._(undefined), which will result in error.

So using it just like i18n._(x) || y isn't possible. We would have to do something like !!x ? (i18n._(x) || x) : x or stick to i18n._(x || y).

himdel · 2021-12-17T03:30:35Z

Well, we could write that as x && i18n._(x) || y, which may be slightly clearer, and maybe we can wrap it in some kind of wrapper function, but I guess no way around it. Luckily it's not in that many places :)

Issue: AAH-1070

himdel

Works for me :)

patchback · 2022-01-27T15:38:27Z

Backport to stable-4.4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4.4/2872781590ab7b8fdcd6a460d0cd016a8a5476cb/pr-1295

Backported as #1563

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* fix repo selector string translating * remove getValFromFnc util * use defineMessage for constant translation * add translation of tasks * fix translation in task_detail page * fix translations * rewrite to clearer statement Issue: AAH-1070 (cherry picked from commit 2872781)

* fix repo selector string translating * remove getValFromFnc util * use defineMessage for constant translation * add translation of tasks * fix translation in task_detail page * fix translations * rewrite to clearer statement Issue: AAH-1070 (cherry picked from commit 2872781) Co-authored-by: Jiří Jeřábek <Jerabekjirka@email.cz>

jerabekjiri requested a review from himdel November 24, 2021 16:18

himdel reviewed Dec 13, 2021

View reviewed changes

src/constants.tsx Outdated Show resolved Hide resolved

himdel reviewed Dec 13, 2021

View reviewed changes

src/containers/task-management/task_detail.tsx Outdated Show resolved Hide resolved

himdel reviewed Dec 13, 2021

View reviewed changes

src/containers/task-management/task_detail.tsx Outdated Show resolved Hide resolved

himdel added the backport-4.4 This PR should be backported to stable-4.4 (2.1) label Dec 14, 2021

himdel mentioned this pull request Jan 11, 2022

Fixed the localization of the Namespaces string #1495

Closed

jerabekjiri added 7 commits January 24, 2022 14:45

fix repo selector string translating

a8948b6

remove getValFromFnc util

e551dd9

use defineMessage for constant translation

30a9f8e

add translation of tasks

9232b1e

fix translation in task_detail page

c4ffbde

fix translations

2e28f31

rewrite to clearer statement

49265cb

Issue: AAH-1070

jerabekjiri force-pushed the fix-repository-selector-translation branch from cb5e034 to 49265cb Compare January 24, 2022 14:28

himdel approved these changes Jan 27, 2022

View reviewed changes

jerabekjiri merged commit 2872781 into ansible:master Jan 27, 2022

patchback bot mentioned this pull request Jan 27, 2022

[PR #1295/28727815 backport][stable-4.4] Fix translation in repo selector #1563

Merged

github-actions bot added the backported-4.4 This PR has been backported to stable-4.4 (2.1) label Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix translation in repo selector #1295

Fix translation in repo selector #1295

jerabekjiri commented Nov 24, 2021 •

edited

jerabekjiri commented Nov 24, 2021

himdel commented Nov 24, 2021 •

edited

himdel commented Nov 24, 2021

jerabekjiri commented Dec 3, 2021

himdel commented Dec 8, 2021

jerabekjiri commented Dec 10, 2021

himdel Dec 13, 2021

jerabekjiri Dec 15, 2021

himdel Dec 13, 2021

himdel commented Dec 13, 2021

himdel commented Dec 13, 2021

jerabekjiri commented Dec 15, 2021

himdel commented Dec 17, 2021 •

edited

himdel left a comment

patchback bot commented Jan 27, 2022 •

edited

	<Tooltip content={i18n._(Constants.TASK_NAMES[name] \|\| name)}>
	<Tooltip content={i18n._(Constants.TASK_NAMES[name]) \|\| name}>

Fix translation in repo selector #1295

Fix translation in repo selector #1295

Conversation

jerabekjiri commented Nov 24, 2021 • edited

jerabekjiri commented Nov 24, 2021

himdel commented Nov 24, 2021 • edited

himdel commented Nov 24, 2021

jerabekjiri commented Dec 3, 2021

himdel commented Dec 8, 2021

jerabekjiri commented Dec 10, 2021

himdel Dec 13, 2021

Choose a reason for hiding this comment

jerabekjiri Dec 15, 2021

Choose a reason for hiding this comment

himdel Dec 13, 2021

Choose a reason for hiding this comment

himdel commented Dec 13, 2021

himdel commented Dec 13, 2021

jerabekjiri commented Dec 15, 2021

himdel commented Dec 17, 2021 • edited

himdel left a comment

Choose a reason for hiding this comment

patchback bot commented Jan 27, 2022 • edited

Backport to stable-4.4: 💚 backport PR created

jerabekjiri commented Nov 24, 2021 •

edited

himdel commented Nov 24, 2021 •

edited

himdel commented Dec 17, 2021 •

edited

patchback bot commented Jan 27, 2022 •

edited