chore(translations): Add missing i18n #17525

hbruch · 2021-11-23T11:03:15Z

SUMMARY

This PR adds the missing *.ts to babel.cfg, so messages in typescript files get extracted.

Addditionally, a few missing i18n calls are added, some not extracted calls (i.e. ${t()} in backticks) rewritten, and the pot file updated.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-11-23T11:24:37Z

Codecov Report

Merging #17525 (9cb3759) into master (2e29f36) will decrease coverage by 0.07%.
The diff coverage is 78.57%.

❗ Current head 9cb3759 differs from pull request most recent head 787009d. Consider uploading reports for the commit 787009d to get more accurate results

@@            Coverage Diff             @@
##           master   #17525      +/-   ##
==========================================
- Coverage   77.01%   76.94%   -0.08%     
==========================================
  Files        1049     1049              
  Lines       56671    56671              
  Branches     7851     7851              
==========================================
- Hits        43648    43606      -42     
- Misses      12770    12812      +42     
  Partials      253      253

Flag	Coverage Δ
hive	`81.60% <ø> (ø)`
mysql	`82.02% <ø> (ø)`
postgres	`82.03% <ø> (ø)`
presto	`?`
python	`82.38% <ø> (-0.15%)`	⬇️
sqlite	`81.71% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/actions/sqlLab.js	`58.69% <0.00%> (ø)`
...c/SqlLab/components/TemplateParamsEditor/index.tsx	`88.88% <ø> (ø)`
...t-frontend/src/dashboard/actions/dashboardState.js	`28.80% <ø> (ø)`
...t-frontend/src/dashboard/components/SliceAdder.jsx	`76.62% <ø> (ø)`
...dashboard/components/SliceHeaderControls/index.tsx	`75.38% <0.00%> (ø)`
...ilters/FilterBar/FilterControls/FilterControls.tsx	`82.69% <ø> (ø)`
...e/components/controls/SelectAsyncControl/index.tsx	`80.48% <0.00%> (ø)`
...c/filters/components/Select/SelectFilterPlugin.tsx	`81.05% <ø> (ø)`
...eModal/DatabaseConnectionForm/CommonParameters.tsx	`94.44% <ø> (ø)`
...set-frontend/src/views/CRUD/welcome/ChartTable.tsx	`73.49% <ø> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e29f36...787009d. Read the comment docs.

villebro

A few first pass comments. Big thanks for this PR, amazing catch!

villebro · 2021-11-23T11:53:59Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

-    return t(LAST_VIEWED, entity.time_delta_humanized);
+    return t('Viewed %s', entity.time_delta_humanized);


I'm not sure this change is necessary - I assume the variable value should be picked up here. Did you check if that's not the case?

I verified, but using constants, the string was not extracted. gilbsgilbs/babel-plugin-i18next-extract#109 seems to confirm this, though the FAQ only speaks of variables, not constants.

In addition, this whole section seems to rely on time_delta_humanized and changed_on_delta_humanized being localized, which apparently is not the case:

For other tables/columns, this seems to be handled differently:

Oh interesting. Maybe I need to switch my local devenv to non-english to catch these, too. It's becoming apparent we need a proper developer tutorial for i18n, as it's obvious this is not being handled consistently across the app.

To avoid duplication and risk of these falling out of sync, maybe we should introduce a similar localized translation mapping as I did in the previous PR:

superset/superset-frontend/src/views/CRUD/welcome/EmptyState.tsx

Lines 25 to 30 in bdc6a1d

const welcomeTableLabels: Record<WelcomeTable, string> = {

[WelcomeTable.Charts]: t('charts'),

[WelcomeTable.Dashboards]: t('dashboards'),

[WelcomeTable.Recents]: t('recents'),

[WelcomeTable.SavedQueries]: t('saved queries'),

};

Regarding time_delta_humanized, I'm pretty sure these are happening in the backend, let me look at those (I have a faint memory of when those were introduced).

I wonder why time_delta_humanized and changed_on_delta_humanized are necessary? moment.fromNow seems to localize pretty well. Aren't time / changed_on / changed_on_utc avaible for every item? Code would become more compact:

const getEntityLastActionOn = (entity: ActivityObject) => { if ('time' in entity) { return t('Viewed %s', moment(entity.time).fromNow()); } let time: number | string | undefined | null; if ('changed_on' in entity) time = entity.changed_on; if ('changed_on_utc' in entity) time = entity.changed_on_utc; return t('Modified %s', time == null ? UNKNOWN_TIME : moment(time).fromNow(),); };

villebro · 2021-11-23T11:54:42Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

-    return t(LAST_MODIFIED, entity.changed_on_delta_humanized);
+    return t('Modified %s', entity.changed_on_delta_humanized);


superset-frontend/src/views/CRUD/welcome/EmptyState.tsx

hbruch · 2021-11-23T21:50:30Z

Another topic for the developer tutorial: ${t('string-to-translate')} in backticks seems not be extracted by babel. Not sure, if this is a bug or expected behavior, which needs to be worked around in superset.

Further, I find some translations in .po files (e.g. Pie Chart in 'sl/LC_MESSAGES/messages.po' or 'zh/LC_MESSAGES/messages.po') which I don't find in the superset project and which are, in consequence, not extracted to the .pot file

Finally, there are a some fragments, where messages are split in multiple sections, because in between there is an HTML element (e.g. ). These are somewhat harder to translate as the fragments on it's own sometimes are not meanigful. Not sure, if there is a better way to handle these.

dkrat7 · 2021-11-24T06:42:21Z

Regarding the additional translations in the 'sl/LC_MESSAGES/messages.po', I included the 'superset-ui' repo in extraction process (if I am right, that won't be problem anymore with monorepo). I also added some strings manually to the po file (I couldn't find the way to extract them with the babel).

Thumbs up for properly solving i18n issues.

…i18n

hbruch · 2021-11-26T11:07:29Z

@villebro I think I need your assistance here. I tried to figure out why the test cases fail but see no causing change in this PR. Wat might be the reason and how could I fix it?

villebro · 2021-11-26T11:33:36Z

@hbruch I think it's just a flaky test. I restarted the test, let's see if that resolves it

villebro

LGTM, awesome work!

villebro · 2021-11-27T12:49:48Z

superset/translations/babel.cfg

@@ -19,6 +19,7 @@
 [jinja2: superset/**/templates/**.html]
 [javascript: superset-frontend/src/**.js]
 [javascript: superset-frontend/src/**.jsx]
+[javascript: superset-frontend/src/**.ts]


@zhaoyongjie let's make sure translations are properly handled after monorepo lands!

@villebro I have rebase master on stage-2 PR

I will add plugins and packages to babel.cfg

Cool thanks @zhaoyongjie !

hbruch added 3 commits November 23, 2021 11:45

Add *.ts to babel.cfg

c3f28de

Add missing i18n calls

a2ebcc2

Update pot file

7a5a1ab

pull-request-size bot added the size/XXL label Nov 23, 2021

villebro reviewed Nov 23, 2021

View reviewed changes

Fix lint issues and review comment

0167d97

villebro reviewed Nov 23, 2021

View reviewed changes

superset-frontend/src/views/CRUD/welcome/EmptyState.tsx Outdated Show resolved Hide resolved

Incorparate review feedback

dc2aeb9

hbruch added 3 commits November 23, 2021 23:37

Fix missing or non-extractable i18n calls

b02d3e0

Update pot file

9e60adb

Fix syntax error in CommonParameters.tsx

755fcff

hbruch added 2 commits November 25, 2021 12:22

Fix introduced issues

75d0990

Merge commit '2e29f36e7885b693a0af2cf8b0b8ca86d2970f64' into missing_…

787009d

…i18n

villebro approved these changes Nov 27, 2021

View reviewed changes

villebro merged commit 6604a6a into apache:master Nov 27, 2021

zhaoyongjie self-requested a review November 27, 2021 13:04

hbruch deleted the missing_i18n branch December 8, 2021 21:40

hbruch mentioned this pull request Dec 27, 2021

Translations files don't translate everything #14848

Closed

CFM90 mentioned this pull request Mar 30, 2022

German Translation not correctly implemented? #19431

Closed

2 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(translations): Add missing i18n #17525

chore(translations): Add missing i18n #17525

hbruch commented Nov 23, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

villebro left a comment

villebro Nov 23, 2021

hbruch Nov 23, 2021

hbruch Nov 23, 2021

villebro Nov 23, 2021

villebro Nov 23, 2021

hbruch Nov 23, 2021

villebro Nov 23, 2021

hbruch commented Nov 23, 2021

dkrat7 commented Nov 24, 2021

hbruch commented Nov 26, 2021

villebro commented Nov 26, 2021

villebro left a comment

villebro Nov 27, 2021

zhaoyongjie Nov 27, 2021

zhaoyongjie Nov 27, 2021

villebro Nov 27, 2021

		return t(LAST_VIEWED, entity.time_delta_humanized);
		return t('Viewed %s', entity.time_delta_humanized);

	const welcomeTableLabels: Record<WelcomeTable, string> = {
	[WelcomeTable.Charts]: t('charts'),
	[WelcomeTable.Dashboards]: t('dashboards'),
	[WelcomeTable.Recents]: t('recents'),
	[WelcomeTable.SavedQueries]: t('saved queries'),
	};

		return t(LAST_MODIFIED, entity.changed_on_delta_humanized);
		return t('Modified %s', entity.changed_on_delta_humanized);

chore(translations): Add missing i18n #17525

chore(translations): Add missing i18n #17525

Conversation

hbruch commented Nov 23, 2021 • edited Loading

SUMMARY

ADDITIONAL INFORMATION

codecov bot commented Nov 23, 2021 • edited Loading

Codecov Report

villebro 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

hbruch commented Nov 23, 2021

dkrat7 commented Nov 24, 2021

hbruch commented Nov 26, 2021

villebro commented Nov 26, 2021

villebro 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

hbruch commented Nov 23, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading