-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore(translations): Add missing i18n #17525
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
A few first pass comments. Big thanks for this PR, amazing catch!
return t(LAST_VIEWED, entity.time_delta_humanized); | ||
return t('Viewed %s', entity.time_delta_humanized); |
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.
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?
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.
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.
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.
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.
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.
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.
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).
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.
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(),);
};
return t(LAST_MODIFIED, entity.changed_on_delta_humanized); | ||
return t('Modified %s', entity.changed_on_delta_humanized); |
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.
same here
Another topic for the developer tutorial: Further, I find some translations in 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. |
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. |
@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? |
@hbruch I think it's just a flaky test. I restarted the test, let's see if that resolves it |
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.
LGTM, awesome work!
@@ -19,6 +19,7 @@ | |||
[jinja2: superset/**/templates/**.html] | |||
[javascript: superset-frontend/src/**.js] | |||
[javascript: superset-frontend/src/**.jsx] | |||
[javascript: superset-frontend/src/**.ts] |
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.
@zhaoyongjie let's make sure translations are properly handled after monorepo lands!
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.
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.
I will add plugins and packages to babel.cfg
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.
Cool thanks @zhaoyongjie !
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 thepot
file updated.ADDITIONAL INFORMATION