-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Update localization from Transifex Brave order #7099
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is incredible!
Would you mind adding the new locales to the locale index in app/_locales/index.json
?
Also, a few messages were removed recently in #7013 : privacyMode
, privacyModeDescription
, approvalData
, and approvalDataDescription
.
@Gudahtt I updated the PR, things are split nicely in commits for easier review. New changes include:
Here's the output of the verify script:
And here's the output of an individual check, which was also broken before:
2 new commits to support these changes weren’t made in Only include translated items if they exist in the en destination file Add English strings if strings are missing |
Thanks! I appreciate the effort in making this easier to review, this is quite helpful.
We fallback to the |
Good catch on the format thing, will update in a couple hours. I'll change it to not include the extra strings from the source locale if you prefer that way too. |
Hmm. I can see how that might be useful. It would necessitate a change to our development workflow though - maybe the addition of some step in CI to prevent us from adding new messages without adding them to all locales, and a tool to help with that. It would also make it more difficult to determine where gaps did exist in translation. I'm certainly open to the idea, but I think it would be best left to a separate PR. |
@Gudahtt good to go, new supporting commit for the conversion script is bbondy/upstream-brave-wallet-strings-to-metamask@31e53c1 |
A few quick changes:
|
There is only 1 entry for |
Oh I mean Tamil/தமிழ் /tml is the same language as the |
Oh ok, I renamed |
@mapmeld OK those changes are done.
|
There are some phrases in old tml which I don't see in the new ta messages; is it possible to merge these in some way? |
- All locales didn't work because it was putting an object inside of an object. - Individual checks didn't work because of how the destructuring was done. - Extra items were being printed as missing items.
Before:
After:
Only a couple of them show down in coverage but that's because unused extra strings that shouldn't be there were removed. |
Looks good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Nothing else has caught my attention.
This is a PR to uplift localized strings in a compatible format for MetaMask.
Since https://github.com/brave/ethereum-remote-client (which is forked from MetaMask) is not always based on the same version as the
develop
branch, I made a small repo here to do the uplifting in a safe way:https://github.com/bbondy/upstream-brave-wallet-strings-to-metamask
It's just a tool that reads the MetaMask locale files and the Brave ones, and puts in any missing string from MetaMask that Brave has.
Note that Brave isn't supporting these locales yet, so no string additions / fixes for those:
hn
,ht
, andph
.This is changing in this PR, but MetaMask was using
tml
forta
which I think was wrong.