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

People: Ensures form header strings are translated properly #369

Merged
merged 3 commits into from
Nov 24, 2015

Conversation

ebinnion
Copy link
Contributor

Addresses part of #138

Previously, the form header title and description were incorrect for a role. The title did not take into account the user of a or an and the explanation was the same for every role.

This PR changes that.

Currently, it is quite verbose, so ping @deBhal for advice on trimming it down a bit.

To test:

  • Checkout update/invite-translations branch
  • Invite a test user to join a WordPress.com site by going to Users > Invite New in wp-admin
  • In the invitation email, make note of the invitation key
  • Go to /accept-invite/$site/$invite_key when you are both logged in and out
  • Do the strings change between logged in and logged out?
  • Now, go back to the invite users UI in wp-admin
  • Invite the same user, but with a different role this time
  • Refresh the tab that has Calypso loaded
  • Do the strings represent the new role?

cc @lezama for an early review

@deBhal
Copy link
Contributor

deBhal commented Nov 22, 2015

Thanks for the ping!

I think this is the good sort of verbose with separate complete sentences for the different roles. Nice one :)

You could streamline some of the calls a bit by moving the siteName up into the siteLink, like <a href={ this.getSiteDomain() } className="invite-header__site-link"> {this.getSiteName()} </a>;. That would let you use replace {{siteLink}}%(siteName)s{{/siteLink}} with {{siteLink/}} and drop the args out of those calls. I think you'd save a few lines, and make it a bit easier for both developers and translators to read.


getLoggedInTitleForInvite() {
let title = '';
let siteLink = (
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be a const

@ebinnion ebinnion force-pushed the update/invite-translations branch from f23b70e to 0cf9899 Compare November 23, 2015 21:06
@ebinnion ebinnion added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Nov 23, 2015
@ebinnion
Copy link
Contributor Author

cc @rickybanister for a wording review.

I based the wording off of the Summary of Roles in the .org codex.

@roccotripaldi
Copy link
Member

Code-wise this looks great. I agree with @deBhal. The verbose strings make this very easy to understand. Tested, and works like a charm. LGTM.

@roccotripaldi
Copy link
Member

Will we deal with the header text in an other PR?

@ebinnion
Copy link
Contributor Author

Yessir. I plan on addressing the strings in InviteHeader in a separate PR.

@lezama
Copy link
Contributor

lezama commented Nov 23, 2015

LGTM

@rickybanister
Copy link

🚢

@ebinnion ebinnion force-pushed the update/invite-translations branch from 0cf9899 to a71ac9f Compare November 24, 2015 17:12
@roccotripaldi roccotripaldi added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 24, 2015
ebinnion added a commit that referenced this pull request Nov 24, 2015
People: Ensures form header strings are translated properly
@ebinnion ebinnion merged commit 7d600ac into master Nov 24, 2015
@ebinnion ebinnion deleted the update/invite-translations branch November 24, 2015 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants