feat(core): add locale and no-translate attributes to html element on startup#16966
feat(core): add locale and no-translate attributes to html element on startup#16966Alexander-Taran wants to merge 1 commit intoeclipse-theia:masterfrom
Conversation
9573c5b to
836242f
Compare
There was a problem hiding this comment.
Thanks for the contribution!
This works well for the main window, but doesn't seem to affect the secondary-window feature - secondary windows are still using lang="en". You can test this by popping out an editor widget for example. Do you mind fixing this as well? Afterwards, this should be good to go.
|
@msujew how would one approach this one? packages/core/src/browser/secondary-window-handler.ts /**
* Sets up message forwarding from the main window to secondary windows.
* Does nothing if this service has already been initialized.
*
* @param shell The `ApplicationShell` that widgets will be moved out from.
* @param dockPanelRendererFactory A factory function to create a `DockPanelRenderer` for use in secondary windows.
*/
init(shell: ApplicationShell, dockPanelRendererFactory: (document?: Document | ShadowRoot) => DockPanelRenderer): void {
if (this.applicationShell) {
// Already initialized
return;
}
this.applicationShell = shell;
this.dockPanelRendererFactory = dockPanelRendererFactory;
this.secondaryWindowService.beforeWidgetRestore(([widget, window]) => this.removeWidget(widget, window));
} |
|
Yes, somewhere here probably, where the new window is instantiated: theia/packages/core/src/browser/secondary-window-handler.ts Lines 176 to 189 in fea3b46 |
836242f to
71f5a49
Compare
|
@msujew |
| protected setNoTranslateOnHtml(): void { | ||
| document.documentElement.setAttribute('translate', 'no'); | ||
| document.documentElement.classList.add('notranslate'); | ||
| } | ||
|
|
||
| protected setLocaleOnHtml(): void { | ||
| const lang = nls.locale?.split('_').at(0) || 'en'; | ||
| document.documentElement.setAttribute('lang', lang); | ||
| } |
There was a problem hiding this comment.
Suggestion: document still refers to the same main document of the application. You probably want to use newWindow.document here instead. This doesn't do anything in its current form.
| protected setNoTranslateOnHtml(): void { | ||
| document.documentElement.setAttribute('translate', 'no'); | ||
| document.documentElement.classList.add('notranslate'); | ||
| } | ||
|
|
||
| protected setLocaleOnHtml(): void { | ||
| const lang = nls.locale?.split('_').at(0) || 'en'; | ||
| document.documentElement.setAttribute('lang', lang); | ||
| } |
There was a problem hiding this comment.
Suggestion: Instead of duplicating the code, I think it'd be better to have a nls.setHtmlLang(element: HTMLElement) (or similar) function available on the nls namespace that sets the translate and lang attributes as well as the notranslate class on the specified element. We can call that for both the main window document as well as any secondary window documents.
There was a problem hiding this comment.
@msujew do I read you right? Combine both calls in respective modules into one calling a function in nls?
Or be more explicit and do 2 calls serving kinda different purposes (set right lang/prevent auto translation)
In a spirit of better overloading I'd have them called in some method of both classes
contribution->setLocaleOnHtmlElement-> nls.boo(); nls.foo()
secondaryWindow -> setLocaleOnHtmlElement -> nls.boo(); nls.foo()
There was a problem hiding this comment.
I'm fine with having two functions for that as well 👍
243fa3e to
c06a247
Compare
startup - Set 'lang' attribute based on detected locale. - Prevent auto-translation by setting 'translate="no"' and adding '.notranslate' class. This ensures better i18n support and avoids issues with unintended UI translation. Signed-off-by: Alexander Taran <a.taran@outlook.com>
c06a247 to
0dc6f4a
Compare
|
@ndoschek I'm currently on vacation and can't check the validity of the latest changes. The code itself looks good to me. Can you assign someone else to give this the green light? Thank you. |
Modify html tag attributes on startup
What it does
This ensures better i18n support and avoids issues with unintended UI translation.
fixes #16968
How to test
Open web application and check that html element in devtools has the right attributes respecting locale saved to localstorage via nls
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers