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

Added context to domain upgrade renewal and expiration strings #15656

Merged
merged 1 commit into from Jun 30, 2017

Conversation

iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Jun 29, 2017

Fixes #248 adding context for the translators when translating some strings. Followed the examples here: https://github.com/Automattic/i18n-calypso#more-translate-examples . With these 3 files we should cover all the cases on domain upgrade renewal / expiration.

I'm not quite sure how to test this locally, as the context will show up on http://translate.wordpress.com/ once is live. Any feedback is appreciated.

@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/248

@matticbot matticbot added the [Size] S Small sized issue label Jun 29, 2017
@iamgabrielma iamgabrielma added [Feature Group] Emails & Domains Features related to email integrations and domain management. i18n [Feature] Purchase Management Related to managing purchases such as subscriptions, plans, history, auto-renew, cancellation, etc. labels Jun 29, 2017
@iamgabrielma iamgabrielma added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 29, 2017
Copy link
Contributor

@aidvu aidvu left a comment

Choose a reason for hiding this comment

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

The linter is probably going to complain. Also, not sure if comment or context: https://github.com/Automattic/i18n-calypso#options

@@ -25,7 +25,7 @@ const MappedDomain = React.createClass( {

if ( domain.isAutoRenewing ) {
return (
<Property label={ translate( 'Mapping renews on' ) }>
<Property label={ translate( 'Mapping renews on', {comment: 'The corresponding date is in a different cell in the UI, the date is not included within the translated string'} ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces after and before curly braces. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Will do! :D

@iamgabrielma
Copy link
Contributor Author

iamgabrielma commented Jun 29, 2017

Also, not sure if comment or context: https://github.com/Automattic/i18n-calypso#options

I'd say is comment because there is not a need for an alternative translation depending on the context, just tells the translators that the date isn't included in the string because is in a different place. Maybe @rachelmcr can clarify this point.

@rachelmcr
Copy link
Member

As @yoavf noted on the issue, this should be added a translator comment, not context, to preserve the existing translations.

@yoavf
Copy link
Contributor

yoavf commented Jun 29, 2017

I'm not quite sure how to test this locally

You can run make translate and look at the calypso-strings.pot file that will be generated.

@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] S Small sized issue labels Jun 29, 2017
@aidvu
Copy link
Contributor

aidvu commented Jun 29, 2017

The rebase probably didn't go as expected. :)

@matticbot matticbot added [Size] S Small sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Jun 30, 2017
@iamgabrielma
Copy link
Contributor Author

The rebase probably didn't go as expected. :)

Definitely, seems that when I tried to fix the ESLint errors, I rebased wrongly and created a divergent branch, that’s why every time I tried to rebase again didn’t work. I've created a new clean branch from the latest master and overwritten this one with the changes via push --force. I hope now works alright now! Thanks for the tips @aidvu

@aidvu aidvu removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 30, 2017
Copy link
Contributor

@aidvu aidvu left a comment

Choose a reason for hiding this comment

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

LGTM

@iamgabrielma iamgabrielma merged commit 049400b into master Jun 30, 2017
@klimeryk klimeryk deleted the fix/248 branch June 30, 2017 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Emails & Domains Features related to email integrations and domain management. [Feature] Purchase Management Related to managing purchases such as subscriptions, plans, history, auto-renew, cancellation, etc. i18n [Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants