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

Correctly internationalize new UI access key placeholder name #574

Merged
merged 3 commits into from
Feb 11, 2020

Conversation

JonathanDCohen
Copy link
Contributor

No description provided.

Copy link
Contributor

@alalamav alalamav left a comment

Choose a reason for hiding this comment

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

Nice fix

@@ -97,7 +97,7 @@
"gcp-firewall-create-3": "Type '0.0.0.0/0' in the 'Source IP ranges' field.",
"gcp-firewall-create-4": "Select 'Allow all' under 'Protocols and ports'.",
"gcp-firewall-create-5": "Click 'Create'.",
"key": "Key",
"key": "Key {keyId}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alalamav Do we need to update the messages in all languages before releasing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will break non-English displayed names for unnamed keys to just say whatever the word is for "key"

Copy link
Contributor

Choose a reason for hiding this comment

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

It would break translations. We have two options: wait for the new string to come out the tranlsation pipeline before importing it, or updating the string for all languages now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, see my below comment. This PR is now a no-op until we send the strings out for translation

Copy link
Collaborator

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Other than the question on waiting on the messages, the change looks good to me.
Thanks for the quick fix Jon!

This simply replicates the existing messages so we can release the Data
Limits UI without waiting for these strings to be translated.
@JonathanDCohen
Copy link
Contributor Author

Added back the key ID to non-English languages to replicate what we're already doing so this won't block the Data Limits UI.
I used ls | grep -v master_messages.* | xargs sed -E -i 's/("key": ".*)"/\1 {keyId}"/', please double check that it didn't mess anything up

Copy link
Contributor

@alalamav alalamav left a comment

Choose a reason for hiding this comment

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

Localized messages LGTM

@JonathanDCohen JonathanDCohen merged commit 8f11f16 into master Feb 11, 2020
@JonathanDCohen JonathanDCohen deleted the cohenjon-localize branch February 11, 2020 21:32
JonathanDCohen pushed a commit that referenced this pull request Feb 19, 2020
* Correctly internationalize new UI access key placeholder name

* Add back the keyID for non-English languages

This simply replicates the existing messages so we can release the Data
Limits UI without waiting for these strings to be translated.

* Specify that keyId will be a number
alalamav added a commit that referenced this pull request Feb 20, 2020
alalamav pushed a commit that referenced this pull request Feb 20, 2020
* Correctly internationalize new UI access key placeholder name

* Add back the keyID for non-English languages

This simply replicates the existing messages so we can release the Data
Limits UI without waiting for these strings to be translated.

* Specify that keyId will be a number
@alalamav alalamav mentioned this pull request Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants