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

chore(settings): remove period in one-line alert bar copy #12957

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Conversation

sardesam
Copy link
Contributor

Because

  • We want to maintain consistency of alert bar copy in account settings

This pull request

  • Removes the period from one line alert bar messages

Issue that this pull request solves

Closes: #9253

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)

N/A

Other information (Optional)

N/A

@sardesam sardesam requested a review from a team as a code owner May 19, 2022 23:13
@LZoog
Copy link
Contributor

LZoog commented May 20, 2022

Huh, I feel like it would be more consistent to add periods where they're missing since there's only a couple spots according to the ticket, the original copy for Settings alert bar messages had periods. It reads weird to me without a period since they're complete sentences and all of these strings have to be relocalized. Can we confirm with Vesta this is what we want to do? I'll defer to her and Wil, just want to call out that I personally think this isn't as nice😅

@sardesam
Copy link
Contributor Author

Thanks, Lauren! I'll contact Vesta to get her input and go from there.

@sardesam sardesam marked this pull request as draft May 20, 2022 15:49
@LZoog
Copy link
Contributor

LZoog commented May 23, 2022

Thanks @sardesam, just to bring some of our Slack convo here - for a recent PR of mine, Vesta confirmed new error message/success message copy and they both had periods, though I didn't specifically call those out. I remember the original Settings copy had periods in the success messaging, so this PR gave me pause due to the number of strings changed; whenever we change them here, they have to be retranslated across all of our locales. It's possible we might be able to do a find-and-replace in the l10n repo to remove these periods to keep the IDs and translations, but I'm not sure how periods work across all locales and we'd have to ask someone from the l10n team if that's possible. Plus this leaves some alert bar messaging with periods right, it's just the single-sentence ones that were updated?

It's fine by me if @clouserw / Vesta want to remove the periods here if this sounds better to them (though do we want to confirm the messages where periods were left?), I just wanted to call out the number of strings this changes vs if we just add them to the couple of spots where they're missing.

@LZoog
Copy link
Contributor

LZoog commented May 23, 2022

@flodolo either way we go here, just so we know for the future - is a punctuation change something that is better handled by creating new FTL IDs as we have done here, or can we save these translations somehow?

@flodolo
Copy link
Contributor

flodolo commented May 23, 2022

@flodolo either way we go here, just so we know for the future - is a punctuation change something that is better handled by creating new FTL IDs as we have done here, or can we save these translations somehow?

Using new IDs is the right approach, as rules for punctuation change by locale.

Pontoon's Translation Memory should help avoid losing too much time retranslating these.

@LZoog
Copy link
Contributor

LZoog commented May 23, 2022

Pontoon's Translation Memory should help avoid losing too much time retranslating these.

Fantastic. Are there any existing docs around how the translation memory works? I'm picturing there being translation suggestions based on a nearly identical string match, that kind of thing would be great to know about or at least have a link to.

@sardesam I may have raised a false alarm here - I know the original Settings copy did have periods so I still personally think it's "more consistent" to change those few that do not have them, but as I've said I defer to Wil/Vesta here. 🙂

@flodolo
Copy link
Contributor

flodolo commented May 24, 2022

Are there any existing docs around how the translation memory works?

Not that I'm aware, but it's also a feature common to all localization management tools
https://en.wikipedia.org/wiki/Translation_memory

In Pontoon, while translating you will see matches from strings already translated. The percentage is the difference between that match and the string you're translating (Levenshtein distance)
https://en.wikipedia.org/wiki/Levenshtein_distance

Schermata 2022-05-24 alle 06 41 20

Because:

* We want to maintain consistency of alert bar copy in account settings

This commit:

* Removes the period from one line alert bar messages

Closes #9253
@sardesam
Copy link
Contributor Author

sardesam commented Jun 6, 2022

@LZoog No false alarm raised. I am glad this issue is getting clarified as it affects both our guidelines and translation process.

On that note, confirmed with Vesta that as per Mozilla's punctuation guidelines, her recommendation is to remove the trailing period on one sentence alerts, and keep it on multi-sentence alerts.

For future reference, in case this ever changes, currently, the lines that contain more than one sentence are:

  1. Thanks! Sharing this data helps us improve Firefox Accounts.
  2. Opt out successful. Firefox Accounts won’t send technical or interaction data to Mozilla.

@sardesam sardesam marked this pull request as ready for review June 7, 2022 21:02
@sardesam sardesam requested a review from a team as a code owner June 7, 2022 21:02
Copy link
Contributor

@bcolsson bcolsson left a comment

Choose a reason for hiding this comment

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

Reviewed all .ftl files, confirmed new IDs for all strings with removed periods.

Copy link
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for dealing with my initial pushback and checking with Vesta 😝

@sardesam sardesam merged commit c2296ea into main Jun 8, 2022
@sardesam sardesam deleted the FXA-3476 branch June 8, 2022 16:33
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.

[FxA-Settings] Messages in green bar are not all followed by “.” (dot)
4 participants