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

Updates from bedrock #375

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Updates from bedrock #375

merged 2 commits into from
Feb 15, 2024

Conversation

MozmarRobot
Copy link
Collaborator

From file changes in mozilla/bedrock@1f1c7fc

@MozmarRobot MozmarRobot requested a review from a team as a code owner February 8, 2024 03:05
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.

@reemhamz

Since we're updating these strings, we should get rid of all the Fluent terms that refer to non Mozilla products.

@reemhamz
Copy link

In this case, since you're referring to the PR removing instances of the IE string, do you mean remove any mention of the IE terms or for all Fluent terms that are not Mozilla products?

@flodolo
Copy link
Contributor

flodolo commented Feb 13, 2024

All non-Mozilla products ({ -brand-name-chrome }, { -brand-name-edge }, { -brand-name-safari }, { -brand-name-opera }, { -brand-name-ie }).

Feel free to tag me in the Bedrock PR for review. Note that you don't need to update the string ID, you can just fix the new strings, since they have not been exposed yet in Pontoon.

@reemhamz
Copy link

I can make that change in a different PR since this one was mainly tackling the IE stuff. I can definitely tag you in that PR when I work on it 👍🏼

Note that you don't need to update the string ID, you can just fix the new strings, since they have not been exposed yet in Pontoon.

Sorry, I think I'm getting a little lost in translation (pun intended) here. Do you mean to just do it the typical way we've been doing it (make the old string obsolete and make a new string ID with -v(#) at the end of it without using those string IDs for the non-Mozilla products?

Also, do we want to update all Fluent strings we have if they have non-Mozilla Fluent terms? Or should this just be the case going forward and to leave the old Fluent strings as they are?

@flodolo
Copy link
Contributor

flodolo commented Feb 13, 2024

Sorry, I think I'm getting a little lost in translation (pun intended) here. Do you mean to just do it the typical way we've been doing it (make the old string obsolete and make a new string ID with -v(#) at the end of it without using those string IDs for the non-Mozilla products?

No, I'm saying NOT to do it the usual way 😉

If a string is already exposed for localization, we need to have it retranslated. That's done by versioning the existing ID (e.g. add -v1), changing the text, and keeping the previous string around as "obsolete" (so we can use that while the new string is being translated).

These strings are stuck in review, so they're not exposed for localization yet. That means that you can update the text directly without versioning the ID.

Also, do we want to update all Fluent strings we have if they have non-Mozilla Fluent terms? Or should this just be the case going forward and to leave the old Fluent strings as they are?

No. We only update strings when we need to make other changes (like in this case), so we can progressively get rid of these terms without asking localizers to retranslate a ton of strings at once.

@reemhamz
Copy link

Thanks for explaining! I'm getting this change done right now. I'll tag you in the PR

Copy link
Contributor

@peiying2 peiying2 left a comment

Choose a reason for hiding this comment

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

LGTM. @flodolo please take another pass of this.

@flodolo flodolo merged commit 6f76f4a into master Feb 15, 2024
2 checks passed
@flodolo flodolo deleted the update-from-bedrock-1f1c7fc5 branch February 15, 2024 05:17
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.

4 participants