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

Close Account: Add context for the button text #25587

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Close Account: Add context for the button text #25587

merged 1 commit into from
Jul 24, 2018

Conversation

akirk
Copy link
Member

@akirk akirk commented Jun 19, 2018

In #24859 we started to use the phrase Close Account in two meanings:

  1. as a headline
  2. as a button label ("action")

This needs differen translations in some languages. Adding a context allows to do this. The Manage Purchases has a similar problem which has already been addressed with a button label context elsewhere, so we're just re-using this.

@akirk akirk added i18n [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging labels Jun 19, 2018
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/close-account-button-context

@akirk akirk requested a review from bluefuton June 19, 2018 13:10
a8ci18n
a8ci18n reviewed Jun 19, 2018
View reviewed changes
client/me/account-close/main.jsx
@@ -192,7 +192,7 @@ class AccountSettingsClose extends Component {
{ hasPurchases &&
! hasAtomicSites && (
<Button primary href="/me/purchases">
{ translate( 'Manage purchases' ) }
{ translate( 'Manage purchases', { context: 'button label' } ) }
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 31 times:
translate( 'Manage Purchases', { context: 'button label'} ) ES Score: 20
See 2 additional suggestions in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

ℹ️ This string already exists with the following contexts:

  • null (no context)
  • button label

Would it make sense to reuse one of the above?

a8ci18n
a8ci18n reviewed Jun 19, 2018
View reviewed changes
client/me/account-close/main.jsx
@@ -181,7 +181,7 @@ class AccountSettingsClose extends Component {
{ ( isLoading || isDeletePossible ) && (
<Button scary onClick={ this.handleDeleteClick }>
<Gridicon icon="trash" />
{ translate( 'Close account' ) }
{ translate( 'Close account', { context: 'button label' } ) }
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 17 times:
translate( 'Close Account' ) ES Score: 13
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

ℹ️ This string already exists without a context. Only add a context if the meaning of the string is very specific.

@alisterscott alisterscott added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 24, 2018
alisterscott
alisterscott approved these changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

@alisterscott alisterscott left a comment

Choose a reason for hiding this comment

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

This LGTM

@alisterscott
Copy link
Contributor

@akirk @bluefuton are we still planning to 🚢 this one?

@bluefuton
Copy link
Contributor

@alisterscott yep! I'll give it a quick rebase and then ship it 🚢

@alisterscott
Copy link
Contributor

yep! I'll give it a quick rebase

I already did

@bluefuton bluefuton force-pushed the fix/close-account-button-context branch from 5fb35da to bf7b3c9 Compare July 24, 2018 00:29
@bluefuton
Copy link
Contributor

I already did

@alisterscott I noticed that created a merge commit (did you use git pull perhaps?) so I did a quick git fetch && git rebase origin/master to bring us back to a single commit.

@alisterscott
Copy link
Contributor

I noticed that created a merge commit (did you use git pull perhaps?)

Yes sorry - thanks for the rebase

@bluefuton bluefuton merged commit 5c64ab3 into master Jul 24, 2018
@bluefuton bluefuton deleted the fix/close-account-button-context branch July 24, 2018 01:32
@matticbot matticbot removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging labels Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a8ci18n a8ci18n a8ci18n left review comments

@alisterscott alisterscott alisterscott approved these changes

@bluefuton bluefuton Awaiting requested review from bluefuton

Assignees
No one assigned
Labels
[Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. i18n
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@akirk @matticbot @alisterscott @bluefuton @a8ci18n