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

New strings for PR#2543 #23

Merged
merged 1 commit into from
Jun 22, 2023
Merged

New strings for PR#2543 #23

merged 1 commit into from
Jun 22, 2023

Conversation

dannycolin
Copy link
Contributor

Please do not merge yet. Waiting on mozilla/multi-account-containers#2543 to get merged in the main repository.

@dannycolin dannycolin requested a review from flodolo as a code owner June 19, 2023 15:17
@dannycolin dannycolin marked this pull request as draft June 19, 2023 15:17
@flodolo
Copy link
Contributor

flodolo commented Jun 19, 2023

Could you provide a screenshot of where these strings are used?

@dannycolin
Copy link
Contributor Author

They're used in Manage Extension Shortcut.

@flodolo
Copy link
Contributor

flodolo commented Jun 19, 2023

Thanks. I see Container Shorcut in the string, but Contain Shortcut X (with X being a number) in the screenshot. Is that the same string concatenated to a number? 🤔

@dannycolin
Copy link
Contributor Author

Is that the same string concatenated to a number?

Yes. AFAIK, it isn't possible to have a string with a placeholder for strings we want to translate in manifest.json.

@flodolo
Copy link
Contributor

flodolo commented Jun 19, 2023

Can these strings be moved out of manifest.json in any way? That's a poor approach for localization.

@dannycolin
Copy link
Contributor Author

Can these strings be moved out of manifest.json in any way? That's a poor approach for localization.

I haven't tested but it might be possible to use a combinaison of browser.runtime.onStartup() and browser.commands.update(). It looks a bit like a hackaround to me but it might be an acceptable compromise.

Copy link
Contributor

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

Couple of nits on the comment, but strings look good. Thanks for removing the hard-coded bits.

en/messages.json Outdated Show resolved Hide resolved
en/messages.json Outdated Show resolved Hide resolved
@dannycolin dannycolin requested a review from flodolo June 21, 2023 18:37
@dannycolin dannycolin marked this pull request as ready for review June 21, 2023 18:38
@dannycolin
Copy link
Contributor Author

On Luke's suggestion, @flodolo can you merge this PR before we merge multi-account-containers#2543 please?

@flodolo flodolo merged commit df0d21e into mozilla-l10n:main Jun 22, 2023
@dannycolin dannycolin deleted the pr-2543 branch August 28, 2024 16:43
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