-
Notifications
You must be signed in to change notification settings - Fork 16
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: add configurable dir
for language directions to app adapter [DHIS2-16480]
#825
Changes from all commits
dc43300
b309510
b05fa8f
9f2f8fa
19cb6d7
c5c06a3
7d25dd2
cbc0d1a
fa0e182
d2faf2a
805e3a6
3f6220a
d4bd3e4
f7df0f2
2e880e2
1d62c34
0c48e4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,162 @@ | ||
import i18n from '@dhis2/d2-i18n' | ||
import moment from 'moment' | ||
|
||
// Init i18n namespace | ||
const I18N_NAMESPACE = 'default' | ||
i18n.setDefaultNamespace(I18N_NAMESPACE) | ||
|
||
/** | ||
* userSettings.keyUiLocale is expected to be formatted by Java's | ||
* Locale.toString(): | ||
* https://docs.oracle.com/javase/8/docs/api/java/util/Locale.html#toString-- | ||
* We can assume there are no Variants or Extensions to locales used by DHIS2 | ||
* @param {Intl.Locale} locale | ||
*/ | ||
const parseJavaLocale = (locale) => { | ||
const [language, region, script] = locale.split('_') | ||
|
||
let languageTag = language | ||
if (script) { | ||
languageTag += `-${script}` | ||
} | ||
if (region) { | ||
languageTag += `-${region}` | ||
} | ||
|
||
return new Intl.Locale(languageTag) | ||
} | ||
|
||
/** | ||
* @param {UserSettings} userSettings | ||
* @returns Intl.Locale | ||
*/ | ||
export const parseLocale = (userSettings) => { | ||
try { | ||
// proposed property | ||
if (userSettings.keyUiLanguageTag) { | ||
return new Intl.Locale(userSettings.keyUiLanguageTag) | ||
} | ||
Comment on lines
+35
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the new property Austin and I proposed, as a proof of concept of forward and backward compatibilty |
||
// legacy property | ||
if (userSettings.keyUiLocale) { | ||
return parseJavaLocale(userSettings.keyUiLocale) | ||
} | ||
} catch (err) { | ||
console.error('Unable to parse locale from user settings:', { | ||
userSettings, | ||
}) | ||
} | ||
|
||
// worst-case fallback | ||
return new Intl.Locale(window.navigator.language) | ||
} | ||
|
||
/** | ||
* Test locales for available translation files -- if they're not found, | ||
* try less-specific versions. | ||
* Both "Java Locale.toString()" and BCP 47 language tag formats are tested | ||
* @param {Intl.Locale} locale | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reworked the below take advantage of |
||
export const setI18nLocale = (locale) => { | ||
const { language, script, region } = locale | ||
|
||
const localeStringOptions = [] | ||
if (script && region) { | ||
localeStringOptions.push( | ||
`${language}_${region}_${script}`, | ||
`${language}-${script}-${region}` // NB: different order | ||
) | ||
} | ||
if (region) { | ||
localeStringOptions.push( | ||
`${language}_${region}`, | ||
`${language}-${region}` | ||
) | ||
} | ||
if (script) { | ||
localeStringOptions.push( | ||
`${language}_${script}`, | ||
`${language}-${script}` | ||
) | ||
} | ||
localeStringOptions.push(language) | ||
|
||
let localeStringWithTranslations | ||
const unsuccessfulLocaleStrings = [] | ||
for (const localeString of localeStringOptions) { | ||
if (i18n.hasResourceBundle(localeString, I18N_NAMESPACE)) { | ||
localeStringWithTranslations = localeString | ||
break | ||
} | ||
unsuccessfulLocaleStrings.push(localeString) | ||
// even though the localeString === language will be the default below, | ||
// it still tested here to provide feedback if translation files | ||
// are not found | ||
} | ||
|
||
if (unsuccessfulLocaleStrings.length > 0) { | ||
console.log( | ||
`Translations for locale(s) ${unsuccessfulLocaleStrings.join( | ||
', ' | ||
)} not found` | ||
) | ||
} | ||
|
||
// if no translation files are found, still try to fall back to `language` | ||
const finalLocaleString = localeStringWithTranslations || language | ||
i18n.changeLanguage(finalLocaleString) | ||
console.log('🗺 Global d2-i18n locale initialized:', finalLocaleString) | ||
} | ||
|
||
/** | ||
* Moment locales use a hyphenated, lowercase format. | ||
* Since not all locales are included in Moment, this | ||
* function tries permutations of the locale to find one that's supported. | ||
* NB: None of them use both a region AND a script. | ||
* @param {Intl.Locale} locale | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New fixes in here to Moment locale loading since the last review, and also split to its own function |
||
export const setMomentLocale = async (locale) => { | ||
const { language, region, script } = locale | ||
|
||
if (locale.language === 'en' || locale.baseName === 'en-US') { | ||
return // this is Moment's default locale | ||
} | ||
|
||
const localeNameOptions = [] | ||
if (script) { | ||
localeNameOptions.push(`${language}-${script}`.toLowerCase()) | ||
} | ||
if (region) { | ||
localeNameOptions.push(`${language}-${region}`.toLowerCase()) | ||
} | ||
localeNameOptions.push(language) | ||
|
||
for (const localeName of localeNameOptions) { | ||
try { | ||
await import( | ||
/* webpackChunkName: "moment-locales/[request]" */ `moment/locale/${localeName}` | ||
) | ||
moment.locale(localeName) | ||
break | ||
} catch { | ||
continue | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Sets the global direction based on the app's configured direction | ||
* (which should be done to affect modals, alerts, and other portal elements). | ||
* Defaults to 'ltr' if not set. | ||
* Note that the header bar will use the localeDirection regardless | ||
*/ | ||
export const setDocumentDirection = ({ localeDirection, configDirection }) => { | ||
// validate config direction (also handles `undefined`) | ||
if (!['auto', 'ltr', 'rtl'].includes(configDirection)) { | ||
document.documentElement.setAttribute('dir', 'ltr') | ||
return | ||
} | ||
|
||
const globalDirection = | ||
configDirection === 'auto' ? localeDirection : configDirection | ||
document.documentElement.setAttribute('dir', globalDirection) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,37 @@ | ||
import { useDataQuery } from '@dhis2/app-runtime' | ||
import i18n from '@dhis2/d2-i18n' | ||
import moment from 'moment' | ||
import { useState, useEffect } from 'react' | ||
import { | ||
setI18nLocale, | ||
parseLocale, | ||
setDocumentDirection, | ||
setMomentLocale, | ||
} from './localeUtils.js' | ||
|
||
export const useLocale = ({ userSettings, configDirection }) => { | ||
const [result, setResult] = useState({ | ||
locale: undefined, | ||
direction: undefined, | ||
}) | ||
|
||
i18n.setDefaultNamespace('default') | ||
|
||
const simplifyLocale = (locale) => { | ||
const idx = locale.indexOf('-') | ||
if (idx === -1) { | ||
return locale | ||
} | ||
return locale.substr(0, idx) | ||
} | ||
|
||
const setGlobalLocale = (locale) => { | ||
if (locale !== 'en' && locale !== 'en-us') { | ||
import( | ||
/* webpackChunkName: "moment-locales/[request]" */ `moment/locale/${locale}` | ||
).catch(() => { | ||
/* ignore */ | ||
}) | ||
} | ||
moment.locale(locale) | ||
|
||
const simplifiedLocale = simplifyLocale(locale) | ||
i18n.changeLanguage(simplifiedLocale) | ||
} | ||
|
||
export const useLocale = (locale) => { | ||
const [result, setResult] = useState(undefined) | ||
useEffect(() => { | ||
if (!locale) { | ||
if (!userSettings) { | ||
return | ||
} | ||
|
||
setGlobalLocale(locale) | ||
setResult(locale) | ||
const locale = parseLocale(userSettings) | ||
|
||
setI18nLocale(locale) | ||
setMomentLocale(locale) | ||
|
||
// Intl.Locale dir utils aren't supported in firefox, so use i18n | ||
const localeDirection = i18n.dir(locale.language) | ||
setDocumentDirection({ localeDirection, configDirection }) | ||
document.documentElement.setAttribute('lang', locale.baseName) | ||
|
||
setResult({ locale, direction: localeDirection }) | ||
}, [userSettings, configDirection]) | ||
|
||
console.log('🗺 Global d2-i18n locale initialized:', locale) | ||
}, [locale]) | ||
return result | ||
} | ||
|
||
|
@@ -47,16 +40,19 @@ const settingsQuery = { | |
resource: 'userSettings', | ||
}, | ||
} | ||
export const useCurrentUserLocale = () => { | ||
// note: userSettings.keyUiLocale is expected to be in the Java format, | ||
// e.g. 'ar', 'ar_IQ', 'uz_UZ_Cyrl', etc. | ||
export const useCurrentUserLocale = (configDirection) => { | ||
const { loading, error, data } = useDataQuery(settingsQuery) | ||
const locale = useLocale( | ||
data && (data.userSettings.keyUiLocale || window.navigator.language) | ||
) | ||
const { locale, direction } = useLocale({ | ||
userSettings: data && data.userSettings, | ||
configDirection, | ||
}) | ||
|
||
if (error) { | ||
// This shouldn't happen, trigger the fatal error boundary | ||
throw new Error('Failed to fetch user locale: ' + error) | ||
} | ||
|
||
return { loading: loading || !locale, locale } | ||
return { loading: loading || !locale, locale, direction } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be nice to add a test for this hook .. I know it's not particularly part of this PR since it didn't exist before, but there is quite a bit of logic here and having a test to validate all the permuations would be nice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That may take me some time since I'm a bit out of practice with mocking things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, we can delay it for later too .. but I didn't mean to test the whole thing, just the useLocale hook with something like react-hooks-testing-library There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay, just the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests added |
||
} |
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.
do we need to set the default to 'ltr' here in case no value was provided?
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.
I suppose that would be good, in case something goes wrong with the env var -- which makes me wonder, should we handle 'default to LTR' here only, and leave out the default env var? 🤔 It would tidy up the env vars slightly in the default case
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.
yeah I would say so .. it would keep the default env vars tidier