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.

Sign up for GitHub

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

Jump to bottom

Adding login template strings for translation #8587

Merged
merged 1 commit into from
May 11, 2021
Merged

Adding login template strings for translation #8587

merged 1 commit into from
May 11, 2021

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Apr 7, 2021

@sg00dwin sg00dwin requested review from spadgett and jhadvig April 7, 2021 22:18
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Apr 7, 2021
sg00dwin
sg00dwin commented Apr 7, 2021
View reviewed changes
frontend/public/locales/en/public.json Outdated
"LoginTitle": "Login",
"WelcomeTo": "Welcome to {{platformTitle}}",
"LogInWith": "Log in with {{providerName}}",
Copy link
Member Author

Choose a reason for hiding this comment

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

@rebeccaalpert I included {{platformTitle}} and {{providerName}} to give the translators context. Is this acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me! Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better if the phrase is the key on the left. The translators only receive the key value and they're not going to be able to translate LoginTitle, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the keys

"Log in to your account": "Log in to your account",
"Log in": "Log in",
"Login": "Login",
"Welcome to {{platformTitle}}": "Welcome to {{platformTitle}}",
"Log in with {{providerName}}": "Log in with {{providerName}}",
"Error": "Error"

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@sg00dwin sg00dwin force-pushed the add-login-strings-for-oauth branch 2 times, most recently from 8632a7f to 2c33549 Compare April 8, 2021 14:10
@sg00dwin sg00dwin mentioned this pull request Apr 8, 2021
Merged
rebeccaalpert
rebeccaalpert approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@rebeccaalpert rebeccaalpert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. It just looks like you need to run yarn i18n. I think the script allows for manual entries. If that turns out to be incorrect and these are overwritten, you may need to add some parser comments somewhere instead like // t('public~Log in').

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rebeccaalpert, sg00dwin
To complete the pull request process, please assign vojtechszocs after the PR has been reviewed.
You can assign the PR to them by writing /assign @vojtechszocs in a comment when ready.

The full list of commands accepted by this bot can be found 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

spadgett
spadgett reviewed Apr 8, 2021
View reviewed changes
frontend/public/locales/en/public.json Outdated
Comment on lines 735 to 736
"Log in with {{providerName}}": "Log in with {{providerName}}",
Copy link
Member

Choose a reason for hiding this comment

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

This won't necessarily work for the OAuth server login page. See openshift/oauth-server#71 (comment)

Copy link
Member Author

@sg00dwin sg00dwin Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@sg00dwin Don't manually add these messages to the public.json file. IIRC yarn i18n will remove them when run. We need to add i18next-parser comments to one our of source files like

Added these parser comments to pod.tsx

spadgett
spadgett requested changes Apr 8, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@sg00dwin Don't manually add these messages to the public.json file. IIRC yarn i18n will remove them when run. We need to add i18next-parser comments to one our of source files like

// t('Log in')
// ...etc...

Then yarn i18n will automatically add them to public.json.

@sg00dwin sg00dwin force-pushed the add-login-strings-for-oauth branch from 2c33549 to c8860f6 Compare April 8, 2021 19:50
@sg00dwin
Copy link
Member Author

sg00dwin commented Apr 9, 2021

/retest

@sg00dwin sg00dwin force-pushed the add-login-strings-for-oauth branch from c8860f6 to 200021c Compare April 12, 2021 14:57
@sg00dwin sg00dwin force-pushed the add-login-strings-for-oauth branch from 200021c to 72b2137 Compare April 27, 2021 14:52
@sg00dwin
Copy link
Member Author

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2021
@sg00dwin sg00dwin force-pushed the add-login-strings-for-oauth branch from 1a9cb63 to 6d4149a Compare May 10, 2021 14:35
@sg00dwin sg00dwin force-pushed the add-login-strings-for-oauth branch from 6d4149a to cb3ebde Compare May 10, 2021 16:57
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2021
spadgett
spadgett approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rebeccaalpert, sg00dwin, spadgett

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

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 11, 2021
@openshift-merge-robot openshift-merge-robot merged commit ddde10b into openshift:master May 11, 2021
@sg00dwin sg00dwin deleted the add-login-strings-for-oauth branch May 11, 2021 17:55
@spadgett spadgett added this to the v4.8 milestone May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebeccaalpert rebeccaalpert rebeccaalpert approved these changes

@spadgett spadgett spadgett approved these changes

@jhadvig jhadvig Awaiting requested review from jhadvig

@jcaianirh jcaianirh Awaiting requested review from jcaianirh

Assignees

@spadgett spadgett

@rebeccaalpert rebeccaalpert

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.8
Development

Successfully merging this pull request may close these issues.
5 participants
@sg00dwin @openshift-ci-robot @spadgett @rebeccaalpert @openshift-merge-robot