-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Translate monaco using default keys #10946
Conversation
...ples/api-samples/src/browser/monaco-editor-preferences/monaco-editor-preference-extractor.ts
Outdated
Show resolved
Hide resolved
...ples/api-samples/src/browser/monaco-editor-preferences/monaco-editor-preference-extractor.ts
Outdated
Show resolved
Hide resolved
I think it's not picking them up because I disabled the rule for the file:
Line 52 in 06aa90c
Following our discussion on the Monaco PR. If you've now fixed the cases that need fixing, you can enable the rule for the file. |
@colin-grant-work Ah, thanks, I didn't notice that. I updated the generator/generated file accordingly. |
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.
The changes look good to me.
Before:
After:
Mostly unrelated to this issue, @msujew, what's the status of translations of plugins? Looking at TS error messages, I get this in VSCode:
and this in Theia:
The TS versions are different, which accounts for some of the difference, but in Theia, it looks like the TS message isn't translated, but in VSCode, it is?
@colin-grant-work Good catch, I actually didn't bother to test with the TypeScript extension. Our I sneaked another change into this PR, if you care you can check this feature again :) |
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.
@msujew I just pushed a commit to fix some of the issues I was having with understanding the code... Reading edit: I also renamed |
@paul-marechal, I somewhat disprefer the change. The new class is setting state based on things that are happening outside of it. Previously, the main class was tracking state based on its progress through the stringification of the list of preferences. @msujew, one option would be to do the stringification of the properties inside the |
@colin-grant-work This should still be the current logic. I'm assuming that the command |
I'm fine with either implementation. @colin-grant-work @paul-marechal If you don't see any critical issues in this PR, I would merge it tomorrow morning, so we definitely get it into the release. |
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.
Code LGTM, the generated file is the same after the new changes.
@msujew you can merge anytime, you don't have to wait tomorrow morning :) |
78d0610
to
2de6870
Compare
What it does
Now that #10736 has been merged, our previous method of translating monaco doesn't work anymore. Instead it can be accomplished way easier :)
Before loading any monaco related code, we simply override the
localize
function to delegate to ournls.localize
function. This doesn't work in all cases (210 of 4200 strings remain untranslated this way), but most of these aren't visible to users anyway. I've made thegetDefaultKey
function a bit more lenient to support labels which have only changed in their capitalization. For example VSCode changed fromPrevious match
toPrevious Match
, which should still be translated correctly.Also updates some generated
nls.localizeByDefault
calls which weren't working correctly. I updated the preference extractor for that. It seems like the eslint rule doesn't pick these up for some reason. Really weird :/I will probably come back to this next month and translate the missing pieces as well.
How to test
Review checklist
Reminder for reviewers