mozilla / blurts-addon

Mozilla Public License 2.0
5 stars 8 forks source link

Issues with strings #120

Closed nhnt11 closed 5 years ago

nhnt11 commented 5 years ago

flod gave me a strings-review on IRC:

3:41:23 PM - flod: fxmonitor.popupText should use %1$S, %2$S, etc. and a proper plural form 3:41:45 PM - flod: the same for the units (million, billion) 3:42:08 PM - flod: accesskeys should be C and D, same case as the label 3:42:22 PM - flod: also the n accesskey

flodolo commented 5 years ago

One more question:

# Firefox Monitor brand string
fxmonitor.brandName=Firefox Monitor

Is Monitor localizable, or should the entire "Firefox Monitor" treated as unlocalizable?

To add more details to the IRC conversation:

# Text content of popup. From left to right, the placeholders are used for:
# 1. The number of accounts compromised in the breach. This may be rounded
#    to the nearest million or billion, see the strings below.
# 2. The name of the breached site.
# 3. The year of the breach.
# 4. The brand name ("Firefox Monitor").
fxmonitor.popupText=%S accounts from %S were compromised in %S. Check %S to see if yours is at risk.

As said, you should use positional arguments (%1$S), and a proper plural form. Since you're creating a stringbundle, I don't think it should be a problem?

It might seem weird, but you need to add the precise localization comment above each of the strings, because it's used by tools to identify plurals and display a different UI.

fxmonitor.popupText.millionUnit=%S million
fxmonitor.popupText.billionUnit=%S billion

Same note here about plurals

fxmonitor.checkButton.label=Check %S
fxmonitor.checkButton.accessKey=c
fxmonitor.dismissButton.label=Dismiss
fxmonitor.dismissButton.accessKey=d
fxmonitor.neverShowButton.label=Never show %S alerts
fxmonitor.neverShowButton.accessKey=n

Access keys are case sensitive, but they will fall back if necessary. In the first one, you'll end up with the lowercase c underlined, in the second it will fall back to D.

nhnt11 commented 5 years ago

@flodolo, Cindy confirmed that we should keep "Firefox Monitor" as a not-to-be-localized string. I will push a PR adding a comment indicating this tonight.

flodolo commented 5 years ago

A couple more things, then we should close this issue.

1) Can you add a CODEOWNER file? See an example here.

2) Can you sync the l10n branch with master?

flodolo commented 5 years ago

This is all done, thanks 👍