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

feat: support dynamic language switching #359

Merged
merged 1 commit into from
Oct 21, 2024
Merged

Conversation

kswenson
Copy link
Member

@kswenson kswenson commented Oct 20, 2024

CODAP v2 required a full page reload to switch language due to SproutCore. CODAP v3 will support dynamic language switching without a page reload, so the CFM needs to as well. With this PR, CFM supports the notion of current language rather than just a default language set on startup. The translation function and the rendering of the menus respect the current language. Since the File menu is provided by the client, there's a new API for replacing the existing menu.

@kswenson kswenson force-pushed the 188428949-switch-language branch from ba94808 to dbff353 Compare October 20, 2024 14:28
@@ -2,10 +2,10 @@
"name": "@concord-consortium/cloud-file-manager",
"description": "Wrapper for providing file management for web applications",
"author": "The Concord Consortium",
"version": "2.0.0-pre.4",
"version": "2.0.0-pre.6",
Copy link
Member Author

Choose a reason for hiding this comment

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

Published pre.5 during testing.

"repository": {
"type": "git",
"url": "https://github.com/concord-consortium/cloud-file-manager"
"url": "git+https://github.com/concord-consortium/cloud-file-manager.git"
Copy link
Member Author

Choose a reason for hiding this comment

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

Recommended during npm publish.

@@ -107,7 +107,7 @@
"publish:npm:next": "npm publish --access public --tag next",
"publish:npm:next:preview": "npm publish --access public --tag next --dry-run",
"publish:yalc": "npx yalc publish",
"strings:build": "./node_modules/.bin/strip-json-comments src/code/utils/lang/en-US-master.json > src/code/utils/lang/en-US.json",
"strings:build": "strip-json-comments src/code/utils/lang/en-US-master.json > src/code/utils/lang/en-US.json",
Copy link
Member Author

Choose a reason for hiding this comment

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

Recommended during npm publish.

@@ -138,8 +138,8 @@ export interface CFMMenuBarOptions {
languageMenu?: {
currentLang: string
options: { label: string, langCode: string }[]
onLangChanged?: (langCode: string) => void
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes the TypeScript type to reflect the pre-existing reality.

@@ -19,21 +19,24 @@ export interface CFMMenuBarOptions {
languageMenu?: {
currentLang: string
options: { label: string, langCode: string }[]
onLangChanged?: (langCode: string) => void
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes the TypeScript type to reflect the pre-existing reality.

}

export interface CFMShareDialogSettings {
serverUrl?: string
serverUrlLabel?: string
}

export interface CFMUIOptions {
menuBar?: CFMMenuBarOptions
export interface CFMUIMenuOptions {
Copy link
Member Author

Choose a reason for hiding this comment

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

Pulled these out into a base type to support the new replaceMenu API.

@@ -97,7 +97,7 @@ class CloudFileManagerUIMenu {
const item = menuItems[i]
if (item === 'separator') {
menuItem = {
key: `seperator${i}`,
key: `separator${i}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing an unrelated typo.

lang = lang.toLowerCase()
let translation = translations[lang] != null ? translations[lang][key] : undefined
if ((translation == null)) { translation = key }
return translation.replace(varRegExp, function(match: string, key: string) {
return Object.prototype.hasOwnProperty.call(vars, key)
? vars[key]
: `'** UKNOWN KEY: ${key} **`
: `'** UNKNOWN KEY: ${key} **`
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing an unrelated typo.

@kswenson kswenson requested a review from dougmartin October 20, 2024 14:30
@kswenson kswenson marked this pull request as ready for review October 20, 2024 14:30
@kswenson kswenson force-pushed the 188428949-switch-language branch from dbff353 to 4ced66c Compare October 20, 2024 16:50
Copy link
Member

@dougmartin dougmartin left a comment

Choose a reason for hiding this comment

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

I think we need to set zh-Hans back to just zh but you might want to test if that is true by setting your browser to zh-CN or zh-HK and seeing if you see Chinese or English.

{key: 'pt-BR', contents: ptBR}, // Brazilian Portuguese
{key: 'th', contents: th}, // Thai
{key: 'tr', contents: tr}, // Turkish
{key: 'zh-Hans', contents: zhHans}, // Simplified Chinese
Copy link
Member

Choose a reason for hiding this comment

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

There is something in the back of my brain that tells me we need to keep it just zh. I think it is because we want any Chinese variant other than Taiwanese to default to Hans which it will do if it is just zh. We don't have to do the same for en-GB/en-CA to use en-US etc because en is the default language used if the specific language requested by the browser is not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if I can figure that out. Other things weren't working with switching due to the code mismatch between CODAP and CFM, so if you're right that zh needs special treatment then we may require some special-case code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do the experiment, but I think it should be fine because both CODAP and CFM use the following code to add languages to the map:

const translations: Record<string, LanguageFileContent> =  {}
languageFiles.forEach(function(lang) {
  translations[lang.key.toLowerCase()] = lang.contents
  // accept full key with region code or just the language code
  const baseLang = getBaseLanguage(lang.key)
  if (baseLang && !translations[baseLang]) {
    translations[baseLang] = lang.contents
  }
})

so zh-Hans will be added to the map under both zh-Hans and zh because the code specifies that in the case of multiple instances of the same language, the first one takes precedence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just ran the experiment. If I set my preferred browser language to simply Chinese, I get zh-Hans.

Copy link
Member

Choose a reason for hiding this comment

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

I remember what it was - this was something in SageModeler when it passed down the language to CODAP in the url. We had to only give it the major language code, I think because CODAP only had zh.

https://github.com/concord-consortium/sage-modeler-site/blob/master/src/code/codap-iframe-src.ts#L20-L24

So I guess no change is needed.

Copy link
Member

@dougmartin dougmartin left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@kswenson kswenson merged commit 77a6ad9 into master Oct 21, 2024
2 checks passed
@kswenson kswenson deleted the 188428949-switch-language branch October 21, 2024 20:41
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