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

Add strings for mozilla/multi-account-containers#1533 #11

Closed
wants to merge 1 commit into from

Conversation

Minigugus
Copy link

mozilla/multi-account-containers#1533 adds localized text to the options tab, then if merged it will need these i18n messages

@@ -351,5 +351,66 @@
},
"integrateContainers": {
"message": "Integrate your Containers with Mozilla VPN"
},
"backupFailure": {
"message": "Something goes wrong, backup failed: $errorMessage$",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this copy coming from? The English sounds off to me.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I'm not a native English speaker and I just copied the text from the linked PR, which was never reviewed. Is something like the suggestion below better?

Suggested change
"message": "Something goes wrong, backup failed: $errorMessage$",
"message": "Something went wrong when backing up containers: $errorMessage$",

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in charge of reviewing content to make sure that the text doesn't have localization issues, before it gets into the localization pipeline. I'm not a good source for the English copy.

There are folks in charge of creating copy for specific Mozilla products, features need to be defined and prioritized. I suggest to wait for guidance in the original PR to check what's the status of the issues you linked to, and have updated copy/UX. Those issues are incredibly old.

}
},
"containersRestored": {
"message": "$restoredCount$ containers restored.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Webextension doesn't support proper plurals, we should stay away from using this type of form (it would fail even for English with 1. Something along the line of Containers restored: $restoredCount$ would scale.

}
},
"containersPartiallyRestored": {
"message": "$restoredCount$ containers restored, but some lost their site association : $incompleteList$.",
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous comments apply to this string (use of plural, source of the copy).

English doesn't use spaces before punctuation, only French does.

},
"containersRestored": {
"message": "$restoredCount$ containers restored.",
"description": "(Options menu) When importing containers from a file succeed",
Copy link
Contributor

Choose a reason for hiding this comment

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

Succeeded, or succeeds. Probably the former.

}
},
"containersRestorationFailed": {
"message": "The file is corrupted, or isn't a container backup file.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Use typographic apostrophes.

Suggested change
"message": "The file is corrupted, or isn't a container backup file.",
"message": "The file is corrupted, or isnt a container backup file.",

},
"saveLegend": {
"message": "Save",
"description": "(Options menu) Save containers to a file group header"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these headers or buttons?

"description": "(Options menu) Validate saving containers to a file"
},
"noteWontBackupCookies": {
"message": "Backups only containers names, icons, colors and site assosiations, but NOT containers' cookies.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides the typo (associations), this looks off as well. Either "backups" should be a verb, and it's misspelled, or the string is missing a verb.

},
"noteWontBackupCookies": {
"message": "Backups only containers names, icons, colors and site assosiations, but NOT containers' cookies.",
"description": "(Options menu) Inform the user that cookies won't be saved and then cannot be restored using the file backup"
Copy link
Contributor

Choose a reason for hiding this comment

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

then -> they?

@flodolo
Copy link
Contributor

flodolo commented Jan 14, 2022

Left this comment originally in the wrong repository, sorry about that.

I might be wrong, but this adds strings that nobody reviewed or approved. The PR was originally created over 2 years ago, a lot has changed in the meantime, and MAC is officially maintained (with processes to create content, devs, etc.).

I suggest to open a pull request for this repository only after the team has looked at the code, and copy has been provided for this feature.

@flodolo flodolo closed this Jan 14, 2022
@Minigugus
Copy link
Author

Minigugus commented Jan 14, 2022

@flodolo Ok, no problem, thanks anyway for the review. I know a lot changed since I created the PR, I'm regularly fixing what's broken to keep up with changes, however no one ever took a look at my PR despite continuous interest from the community (mozilla/multi-account-containers#1533 (comment)), and I don't know who to contact for the PR to be reviewed. This is a really small feature, I don't really understand why nobody at Mozilla ever bothered with it. Do you have any recommendations on this?

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.

2 participants