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

copy(fxa-auth-server): update email copy verificationReminderFirst #13924

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

millmason
Copy link
Contributor

Because

  • We're updating language in emails

This pull request

  • Updates email copy strings and their corresponding localization IDs

Issue that this pull request solves

Closes: # n/a

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Screen Shot 2022-08-10 at 12 29 25 PM

@millmason millmason marked this pull request as ready for review August 10, 2022 19:59
@millmason millmason requested review from a team as code owners August 10, 2022 19:59
verificationReminderFirst-title-2 = Welcome to { -brand-firefox }!
verificationReminderFirst-description-2 = A few days ago you created a { -product-firefox-account }, but never confirmed it. Please confirm your account within the next 15 days or it will be automatically deleted.
verificationReminderFirst-sub-description-2 = Don’t miss out on tech that puts you and your privacy first.
confirm-email-2 = Confirm account:
Copy link
Contributor

Choose a reason for hiding this comment

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

confirm-email-2 contains a colon, but looking at the mock-up this appears to be the button text. Should the colon be removed?

Or is verificationReminderFirst-action-2 the button text? If so, I'm not quite sure about the context where this appears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of an odd pattern to me, too -- it looks like it's just a variable that gets assigned into confirm-email-plaintext-2 below. There seems to be a pattern (also visible in the verificationReminderSecond email, for example) where in the standard version of the email, you get button with the CTA (sans colon), and then in the plaintext version, you get the same CTA, but as text followed by a colon, followed by a link. Like this (seen in storybook):
Screen Shot 2022-08-10 at 2 19 06 PM
Screen Shot 2022-08-10 at 2 19 08 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, but if that's the case then wouldn't having Confirm email: being called into { confirm-email-2 }: then result in Confirm email:: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Yeah it totally would. I'm so sorry I missed that, updating now

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Looks good now 👍

@millmason millmason force-pushed the FXA-5206-verificationReminderFirst-copy branch from 8beca32 to 7569d18 Compare August 10, 2022 22:06
@millmason millmason force-pushed the FXA-5574-copy-changes branch 3 times, most recently from b88cfd9 to b46f327 Compare August 10, 2022 22:40
@millmason millmason force-pushed the FXA-5206-verificationReminderFirst-copy branch from 7569d18 to 33a3133 Compare August 11, 2022 18:50
@millmason millmason changed the base branch from FXA-5574-copy-changes to main August 11, 2022 18:52
@millmason millmason force-pushed the FXA-5206-verificationReminderFirst-copy branch from 33a3133 to 38f0854 Compare August 11, 2022 19:07
verificationReminderFirst-description-2 = A few days ago you created a { -product-firefox-account }, but never confirmed it. Please confirm your account within the next 15 days or it will be automatically deleted.
verificationReminderFirst-sub-description-2 = Don’t miss out on tech that puts you and your privacy first.
confirm-email-2 = Confirm account
confirm-email-plaintext-2 = { confirm-email-2 }:
Copy link
Contributor

Choose a reason for hiding this comment

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

@bcolsson does : mean the same thing in all languages when used this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm....I just did a search and this is the only reference to confirm-email-2 in the verificationReminderFirst directory. I'm not aware of a reason why we couldn't just include the : in the string (which could solve this issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

@chenba
Great question, thanks for asking!
Typically, yes, although different languages may need to adjust the structure slightly. For example French might need to insert a non breaking space before the colon. Even if the meaning were different since the colon is part of the string (and thus can be changed when translated), localizers are able to adjust the string to work in their languages.
Also of note, that a similar string structure already exists out in the wild (e.g. verifyPrimary-action-plaintext) so we should be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I don't think I'm reading this correctly. The localiser can still use another symbol (and in a different location relative to confirm-email-2); they just can't break up confirm-email-2 or translate it differently. So I think we are ok here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, Bryan. (Saw your response after I posted my above comment.)

@millmason millmason merged commit c122110 into main Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants