Skip to content
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

Support multi locales in one translation file #4814

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

shu-mutou
Copy link
Contributor

@shu-mutou shu-mutou commented Jan 21, 2020

zh is same as zh-cn, so maintain zh and change zh-cn as link to zh.

Also, added support for i18n/locales_not_for_build_local that keeps locales not for build as local setting for developer. Because building dashboard takes much time.

Ref: #4734 comment
Ref: #4763

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 21, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2020
@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #4814 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4814      +/-   ##
==========================================
- Coverage   45.43%   45.42%   -0.02%     
==========================================
  Files         214      214              
  Lines        9977     9977              
  Branches       94       94              
==========================================
- Hits         4533     4532       -1     
- Misses       5180     5181       +1     
  Partials      264      264
Impacted Files Coverage Δ
...p/backend/integration/metric/common/aggregation.go 89.09% <0%> (-1.82%) ⬇️

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 998f3aa...f093ae8. Read the comment docs.

@floreks
Copy link
Member

floreks commented Jan 27, 2020

I am not sure if creating a symbolic link is a correct way of doing this. Every translation file in the header has information about target-language

<file source-language="en" datatype="plaintext" original="ng2.template" target-language="zh-cn">

If we create a symbolic link to a different language code, then target-language will not be correct.

@shu-mutou
Copy link
Contributor Author

shu-mutou commented Jan 29, 2020

We can use zh-Hans representing Simplified Chinese and and zh-Hant for Traditional Chinese.
http://www.iana.org/assignments/lang-tags/zh-Hans
http://www.iana.org/assignments/lang-tags/zh-Hant

Firefox and Chrome don't seem to have zh-Hans and zh-Hant, but we can create them for translation work and link to them from other zh*s.

It has inconsistent in target-language but reasonable if do so, I think. npm builds dashboard for each languages with symbolic linked messages.xx.xlf file and the binary works perfectly as expected.

I'll investigate how not to build zh-hans and zh-hant that won't be used, and update document.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2020
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. language/zh Updates or issues for Chinese translations. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 29, 2020
@shu-mutou
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2020
@hwdef
Copy link
Member

hwdef commented Jan 29, 2020

This may not be the right way.
Although both mainland China and Singapore use simplified Chinese, some words are different.
Although Taiwan and Hong Kong both use traditional Chinese, they are also very different.
So we should use different files for each region

@shu-mutou
Copy link
Contributor Author

/hold
@hwdef I see situations for zh locales. I'll revert zh* reconstructions. Thanks.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2020
@shu-mutou
Copy link
Contributor Author

@hwdef, how about zh and zh-cn ? Are there deference between them?

@hwdef
Copy link
Member

hwdef commented Jan 29, 2020

@shu-mutou
zh represents all kinds of Chinese, it is better to make zh consistent with zh-cn, because zh-cn is the most used.

@shu-mutou
Copy link
Contributor Author

@hwdef I got it. I'll make zh-cn as link to zh.

`zh` is same as `zh-cn`, so maintain `zh` and change `zh-cn` to link to `zh`.

Also, added support for `i18n/locales_not_for_build_local` that keeps
locales not for build as local setting for developer. Because building
dashboard takes much time.
@shu-mutou
Copy link
Contributor Author

@floreks Changes for this PR are coming back to first situation and inconsistency in target-language is back. But zh means "all kinds of Chinese" and its contents is same as zh-cn, as hwdef said, so it's reasonable doing so, I think.

@shu-mutou
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2020
@floreks
Copy link
Member

floreks commented Jan 29, 2020

I am not fully sure if keeping target locale that is different than configured locale code will not cause some issues during compiling the translated versions. Having multiple overlapping target languages might cause issues.

@shu-mutou
Copy link
Contributor Author

@floreks No worry, I think. Dashboard is built with ng build --i18nLocale = [locale] and the Angular documentation says that the locale specified by this option will be set to the LOCALE_ID of the compiled file.
https://github.com/angular/angular/blob/master/aio/content/guide/i18n.md#i18n-pipes

In fact, LOCALE_ID in compiled JavaScript files for zh-cn that created as a symbolic link tozh in this PR is zh-cn instead of zh.
As same as zh-cn, compiled files for ja or fr that do not have target-language in xlf files have each LOCALE_ID specified with --i18nLocale option.

@floreks
Copy link
Member

floreks commented Jan 30, 2020

@shu-mutou makes sense.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, shu-mutou

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit e30341a into kubernetes:master Jan 30, 2020
@shu-mutou shu-mutou deleted the i18n-multi-locale branch February 4, 2020 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/zh Updates or issues for Chinese translations. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants