-
Notifications
You must be signed in to change notification settings - Fork 490
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
Add Chinese translations for Actions on the service page #7757
Conversation
Thanks @Chenrujie-85 for the PR! Nice contribution to extend Kiali internationalization 🙂 I will review it soon. |
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.
@Chenrujie-85 the new entries are not ordered alphabetically. Could you execute frontend/yarn build
? The build command will re-generate the translation.json
file with ordered entries.
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.
Ok, I estimate that I will complete this task from 9am to 10am Beijing time
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 ran yarn build and pushed the new translation.json, which is now ordered alphabetically.
* 'master' of https://github.com/kiali/kiali: obtain the grafana and tracing version from the internal in-cluster url (kiali#7746) Adapt cypress tests to OSSMC (kiali#7656) Follow-on work to the waypoint service injection work, this looks (kiali#7756) Prevent service node injection for waypoint edges (kiali#7742) Update serialize-javascript dependency (kiali#7750) Update express dependency (again) (kiali#7738) [make] bump some olm stuff (kiali#7748) Update install istio script to make arg optional (kiali#7743) Update bookinfo install hack script such that for ambient the two "db" (kiali#7736) Upgrade images for IBM (kiali#7707) Update express, path-to-regexp and micromatch dependencies (kiali#7730) Added missing K8sGateway label name check for MissingSidecar icon (kiali#7721) Istio config list: loop call validations per namespace in backend (kiali#7688) Upgrade Patternfly to version 5.4 (kiali#7676) # Conflicts: # frontend/public/locales/zh/translation.json
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.
Great job @Chenrujie-85! Overall is quite good, only some comments about translating external variables (WIZARD_TITLES
) and bold texts.
@@ -181,7 +182,7 @@ export class GatewaySelector extends React.Component<Props, GatewaySelectorState | |||
id="includeMesh" | |||
label={ | |||
<> | |||
Include <b>mesh</b> gateway | |||
{t('Include')} <b>mesh</b> {t('gateway')} |
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.
In this case, I think it is better to have the entire statement translated rather than just the bold word mesh
. In other languages maybe the word order is not the same.
{t('Include')} <b>mesh</b> {t('gateway')} | |
{t('Include mesh gateway')} |
@@ -103,13 +104,13 @@ export type WizardAction = | |||
export type WizardMode = 'create' | 'update'; | |||
|
|||
export const WIZARD_TITLES = { |
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.
Adding the t
function to an external variable does not translate the variable. This is only used to generate the translation statements in translation.json
file. For external variables not defined within a React component you have to perform an extra step: adding the t
function to the WIZARD_TITLES
variable in the Kiali application.
${t(WIZARD_TITLES[this.props.type])}
I have added some suggestions. Look for all the WIZARD_TITLES variables used and add them the t
function.
? `${t('Update')} ${WIZARD_TITLES[this.props.type]}` | ||
: `${t('Create')} ${WIZARD_TITLES[this.props.type]}` |
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.
Add the t
function to the WIZARD_TITLES
variable to be translated (explained in another comment):
? `${t('Update')} ${WIZARD_TITLES[this.props.type]}` | |
: `${t('Create')} ${WIZARD_TITLES[this.props.type]}` | |
? `${t('Update')} ${t(WIZARD_TITLES[this.props.type])}` | |
: `${t('Create')} ${t(WIZARD_TITLES[this.props.type])}` |
? `${t('Update')} ${WIZARD_TITLES[this.props.type]}` | ||
: `${t('Create')} ${WIZARD_TITLES[this.props.type]}` |
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.
? `${t('Update')} ${WIZARD_TITLES[this.props.type]}` | |
: `${t('Create')} ${WIZARD_TITLES[this.props.type]}` | |
? `${t('Update')} ${t(WIZARD_TITLES[this.props.type])}` | |
: `${t('Create')} ${t(WIZARD_TITLES[this.props.type])}` |
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 have corrected the error you mentioned. Please review it when you have time. Thank you very much.
[WIZARD_REQUEST_TIMEOUTS]: t('Request Timeouts'), | ||
[WIZARD_K8S_REQUEST_ROUTING]: t('K8s HTTP Routing'), | ||
[WIZARD_K8S_GRPC_REQUEST_ROUTING]: t('K8s GRPC Routing') | ||
[WIZARD_REQUEST_ROUTING]: 'Request Routing', |
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.
The t
funciton is needed here as well. If you remove it, the literal statements won't be included in translation.json
file.
In summary, the t
function is used twice for external variables:
- In the definition of the external variable, to include the literal statements to
translation.json
file - In the variable usage within the React component, to translate the variable.
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.
Thank you for your review. I've corrected the problem.
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.
Thanks for the quick fix. There are 2 small additional changes, not your fault but it would be great to have them in the PR.
@@ -70,7 +71,7 @@ export class AbortFault extends React.Component<Props> { | |||
/> | |||
<FormHelperText> | |||
<HelperText> | |||
<HelperTextItem>{isValid(this.props.isValid) ? httpStatusMsg : httpStatusMsg}</HelperTextItem> | |||
<HelperTextItem>{isValid(this.props.isValid) ? t(httpStatusMsg) : t(httpStatusMsg)}</HelperTextItem> |
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.
This is not your fault, but it doesn't make sense to show the same message for both true and false.
<HelperTextItem>{isValid(this.props.isValid) ? t(httpStatusMsg) : t(httpStatusMsg)}</HelperTextItem> | |
<HelperTextItem>{t(httpStatusMsg)}</HelperTextItem> |
@@ -67,7 +68,7 @@ export class RouteRetry extends React.Component<RouteRetryProps> { | |||
/> | |||
<FormHelperText> | |||
<HelperText> | |||
<HelperTextItem>{isValid(this.props.isValidRetry) ? tryTimeoutMsg : tryTimeoutMsg}</HelperTextItem> | |||
<HelperTextItem>{isValid(this.props.isValidRetry) ? t(tryTimeoutMsg) : t(tryTimeoutMsg)}</HelperTextItem> |
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.
the same here.
<HelperTextItem>{isValid(this.props.isValidRetry) ? t(tryTimeoutMsg) : t(tryTimeoutMsg)}</HelperTextItem> | |
<HelperTextItem>{t(tryTimeoutMsg)}</HelperTextItem> |
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.
As you suggested, the code here should indeed be modified. I have completed the modification, please review it when you have time
Signed-off-by: CTakanashiRikka <[email protected]>
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.
Good job @Chenrujie-85! I have tested the PR and it looks great! You have added many i18n labels to the Kiali application. I just have one final request before approving the PR. Please re-generate the translation.json
file by running frontend/yarn build
to remove deprecated entries. Also there are some conflicts to be addressed.
"FQDN": "完全限定域名", | ||
"gateway": "网关", |
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.
This entry does not longer exist in the application. Please re-generate the translation.json
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.
Ok, I re-run yarn build and submitted the updated translation.json.
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 think I have resolved the conflict. Please review it for me when you have time. If you have any questions, please continue to raise them and I will continue to improve them.
* origin/master: Add Chinese translation
* 'master' of https://github.com/kiali/kiali: Show workload action list in kiosk mode (kiali#7752) rename in_cluster_url to internal_url; rename url to external_url (kiali#7759) Add new cypress test (kiali#7763) [hack] support Sail on vanilla Kubernetes now that OperatorHub.io has the Sail operator (kiali#7761) [Ambient] Add bi-directional edge support for waypoint traffic (kiali#7719) # Conflicts: # frontend/public/locales/zh/translation.json # frontend/src/components/IstioWizards/WorkloadWizardDropdown.tsx
* origin/master: # Conflicts: # frontend/src/components/IstioWizards/WorkloadWizardDropdown.tsx
Thanks for solving the conflicts @Chenrujie-85. I have created a small PR to your PR since some of the labels you translated have been moved to a new component |
Add i18n labels to WorkloadWizardActionsDropdownGroup
I have merged your PR, thank you very much for your help |
Do I need to pass all the tests to be merged? |
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. All tests passed.
Thank you very much for your contribution @Chenrujie-85! I'm looking forward to seeing more Chinese translations in other areas of the Kiali application in the future.
Thank you very much for your help in the process, and I will continue to add Chinese translations in kiali in the future. |
Describe the change
Add Chinese translations for Actions on the service page
Steps to test the PR
Automation testing
If applicable, explain the case scencarios covered by unit / integration / e2e tests created for this PR.
Issue reference
#7737