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

French translation #3770

Merged
merged 44 commits into from
May 12, 2019
Merged

French translation #3770

merged 44 commits into from
May 12, 2019

Conversation

feloy
Copy link
Member

@feloy feloy commented Apr 28, 2019

Fixes #3574

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 28, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 28, 2019
@codecov
Copy link

codecov bot commented Apr 28, 2019

Codecov Report

Merging #3770 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3770      +/-   ##
==========================================
+ Coverage    47.1%   47.13%   +0.02%     
==========================================
  Files         171      171              
  Lines        7971     7971              
  Branches       62       62              
==========================================
+ Hits         3755     3757       +2     
+ Misses       3976     3973       -3     
- Partials      240      241       +1
Impacted Files Coverage Δ
...p/backend/integration/metric/common/aggregation.go 89.09% <0%> (-1.82%) ⬇️
src/app/backend/sync/secret.go 73.26% <0%> (+2.97%) ⬆️

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 f8c18c3...d8e8f92. Read the comment docs.

@feloy feloy force-pushed the feloy-i18n branch 2 times, most recently from cc6b26c to 92b4d66 Compare May 2, 2019 13:14
@maciaszczykm
Copy link
Member

Let us know when you would like to receive some feedback/review.

@feloy
Copy link
Member Author

feloy commented May 2, 2019

Feedbacks are welcome at this point. Thanks

Let us know when you would like to receive some feedback/review.

@@ -13,7 +13,7 @@
"start:frontend:https": "node aio/scripts/version.js && ng serve --progress=false --aot --proxy-config aio/https-proxy.conf.json --ssl --host=0.0.0.0",
"start:backend": "gulp serve --kubeconfig $npm_package_config_kubeconfig",
"start:backend:https": "gulp serve --kubeconfig $npm_package_config_kubeconfig --autoGenerateCerts true --defaultCertDir $npm_package_config_kubeconfig_dir",
"start:prod": "npm run build && ./$npm_package_config_dashboard_binary_path --kubeconfig $npm_package_config_kubeconfig",
"start:prod": "npm run build && (cd $(dirname ./$npm_package_config_dashboard_binary_path) && ./$(basename $npm_package_config_dashboard_binary_path) --kubeconfig $npm_package_config_kubeconfig)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cd looks ugly here. Can we avoid using it from npm scripts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose is to exec the process from its own directory, because the file ./locale_conf.json is referenced in the code.

I did not find the definition of the $npm_package_config_dashboard_binary_path variable, where is it defined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is defined in .npmrc.

@@ -15,11 +15,13 @@
-->

<div class="kd-notifications-container">
<span style="display: none"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to not use styles from templates. Why do you need it? What are these 3 lines doing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content is used some lines below, for the matTooltip translation. Unfortunately, there is a current limitation in the i18n angular code and it is not possible to use ICU ({VAR, select, ...}) in a i18n-matTooltip (or any i18n-*), so we need to create an invisible element for that.

It seems it is not possible either to use an ng-template element.

@ocombe do you have some advice on this problem?

Copy link

@ocombe ocombe May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So most likely the content of i18n-matTooltip is used by the directive directly and is not analyzed as template code, which is why the ICU doesn't work?
ng-template should work though

you can also use ng-container to create an invisible element

Copy link
Member Author

@feloy feloy May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the exact description of the problem: angular/angular#21615

And an explanation of the hack I'm using: http://chrisgriffing.com/coding/angular/2017/07/22/angular-i18n-icu-messages-attributes/

Unfortunately, that does not work with ng-container (the content is not invisible) or ng-template (the textContent is empty)

I've replaced the style="display: none" with an hidden attribute

@@ -35,10 +35,11 @@
class="kd-card-actions kd-muted">
<ng-content select="[actions]"></ng-content>
</div>

<span style="display: none"
#translatedTooltip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@feloy feloy changed the title WIP: French translation French translation May 4, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2019
@feloy feloy changed the title French translation WIP: French translation May 4, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2019
@feloy feloy changed the title WIP: French translation French translation May 4, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2019
@feloy
Copy link
Member Author

feloy commented May 4, 2019

I have finished the translation of the templates.

I will have to look at the MSG_ and figure out how to translate from the TS code (probably in another pull request).

@maciaszczykm
Copy link
Member

Please rebase/merge master to fix the issues with the CI job. Sorry for the inconvenience.

@jeefy
Copy link
Member

jeefy commented May 12, 2019

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy, jeefy

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2019
@k8s-ci-robot k8s-ci-robot merged commit e0420e4 into kubernetes:master May 12, 2019
@floreks
Copy link
Member

floreks commented May 14, 2019

@jeefy @feloy @maciaszczykm

I don't want to revert it because overall it is really good work, but I'd address 2 things.

  1. Remove usage of the hidden element as a workaround for tooltip message logic. Export the logic to a function and when angular will support translations from the ts/js code also we will use that.
  2. Definitely remove cd ... logic from the package.json. We have to keep it simple. Building a translated version was already supported by our build scripts for prod.

@feloy
Copy link
Member Author

feloy commented May 14, 2019

I will work on this asap

@feloy feloy mentioned this pull request May 14, 2019
@feloy
Copy link
Member Author

feloy commented May 14, 2019

@maciaszczykm I have proposed a patch on #3812

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. 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.

Support i18n
6 participants