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: add configurable dir for language directions to app adapter [DHIS2-16480] #825

Merged
merged 17 commits into from
Jan 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions adapter/src/components/AppWrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,15 @@ import { ErrorBoundary } from './ErrorBoundary.js'
import { LoadingMask } from './LoadingMask.js'
import { styles } from './styles/AppWrapper.style.js'

const AppWrapper = ({ children, plugin, onPluginError, clearPluginError }) => {
const { loading: localeLoading } = useCurrentUserLocale()
const AppWrapper = ({
children,
plugin,
onPluginError,
clearPluginError,
direction: configDirection,
}) => {
const { loading: localeLoading, direction: localeDirection } =
useCurrentUserLocale(configDirection)
const { loading: latestUserLoading } = useVerifyLatestUser()

if (localeLoading || latestUserLoading) {
Expand Down Expand Up @@ -40,7 +47,9 @@ const AppWrapper = ({ children, plugin, onPluginError, clearPluginError }) => {
return (
<div className="app-shell-adapter">
<style jsx>{styles}</style>
<ConnectedHeaderBar />
<div dir={localeDirection}>
<ConnectedHeaderBar />
</div>
<div className="app-shell-app">
<ErrorBoundary onRetry={() => window.location.reload()}>
{children}
Expand All @@ -54,6 +63,7 @@ const AppWrapper = ({ children, plugin, onPluginError, clearPluginError }) => {
AppWrapper.propTypes = {
children: PropTypes.node,
clearPluginError: PropTypes.func,
direction: PropTypes.oneOf(['ltr', 'rtl', 'auto']),
plugin: PropTypes.bool,
onPluginError: PropTypes.func,
}
Expand Down
3 changes: 3 additions & 0 deletions adapter/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const AppAdapter = ({
appVersion,
url,
apiVersion,
direction,
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

pwaEnabled,
plugin,
parentAlertsAdd,
Expand Down Expand Up @@ -41,6 +42,7 @@ const AppAdapter = ({
plugin={plugin}
onPluginError={onPluginError}
clearPluginError={clearPluginError}
direction={direction}
>
{children}
</AppWrapper>
Expand All @@ -56,6 +58,7 @@ AppAdapter.propTypes = {
apiVersion: PropTypes.number,
children: PropTypes.element,
clearPluginError: PropTypes.func,
direction: PropTypes.oneOf(['ltr', 'rtl', 'auto']),
parentAlertsAdd: PropTypes.func,
plugin: PropTypes.bool,
pwaEnabled: PropTypes.bool,
Expand Down
87 changes: 70 additions & 17 deletions adapter/src/utils/useLocale.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,45 @@ import i18n from '@dhis2/d2-i18n'
import moment from 'moment'
import { useState, useEffect } from 'react'

i18n.setDefaultNamespace('default')
const I18N_NAMESPACE = 'default'
i18n.setDefaultNamespace(I18N_NAMESPACE)

const simplifyLocale = (locale) => {
const idx = locale.indexOf('-')
if (idx === -1) {
const transformJavaLocale = (locale) => {
return locale.replace('_', '-')
}

// if translation resources aren't found for the given locale, try shorter
// versions of the locale
// e.g. 'pt_BR_Cyrl_asdf' => 'pt_BR', or 'ar-NotFound' => 'ar'
const validateLocaleByBundle = (locale) => {
if (i18n.hasResourceBundle(locale, I18N_NAMESPACE)) {
return locale
}

console.log(`Translations for locale ${locale} not found`)

// see if we can try basic versions of the locale
// (e.g. 'ar' instead of 'ar_IQ')
const match = /[_-]/.exec(locale)
if (!match) {
return locale
}
return locale.substr(0, idx)

const separator = match[0] // '-' or '_'
const splitLocale = locale.split(separator)
for (let i = splitLocale.length - 1; i > 0; i--) {
const shorterLocale = splitLocale.slice(0, i).join(separator)
if (i18n.hasResourceBundle(shorterLocale, I18N_NAMESPACE)) {
return shorterLocale
}
console.log(`Translations for locale ${shorterLocale} not found`)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

for this logic for getting the shorter version, instead of doing it manually by splitting .. couldn't we use the Locale methods, i.e.

const locale = new Intl.Locale('ja-Jpan-JP');

console.log(locale.language, locale.region, locale.script);
// will give us language = ja, region = JP, script = Jpan
// and we can try to get the locale for language or `${language}-${region}``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah that's a nice helper function -- if that works, I think that can help make that code look a lot more semantic 👍 Implementing it is a bit complex given the existing translation files we have to work with, for example uz_UZ_Cyrl isn't in the order that Intl.Locale wants, but I'll see what I can do (when using hyphens, new Intl.Locale('uz-UZ-Cyrl') throws an error)

Here's a screenshot from the Dashboard app's locales:

Screenshot 2024-01-19 at 1 42 58 PM

// if nothing else works, use the initially provided locale
return locale
}

// Set locale for Moment and i18n
const setGlobalLocale = (locale) => {
if (locale !== 'en' && locale !== 'en-us') {
import(
Expand All @@ -23,22 +52,41 @@ const setGlobalLocale = (locale) => {
}
moment.locale(locale)

const simplifiedLocale = simplifyLocale(locale)
i18n.changeLanguage(simplifiedLocale)
const resolvedLocale = validateLocaleByBundle(locale)
kabaros marked this conversation as resolved.
Show resolved Hide resolved
i18n.changeLanguage(resolvedLocale)

console.log('🗺 Global d2-i18n locale initialized:', resolvedLocale)
}

// Sets the global direction based on the app's configured direction
// (which should be done to affect modals, alerts, and other portal elements),
// then returns the locale's direction for use on the header bar
const handleDirection = ({ locale, configDirection }) => {
kabaros marked this conversation as resolved.
Show resolved Hide resolved
// for i18n.dir, need JS-formatted locale
Copy link
Contributor

Choose a reason for hiding this comment

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

Also for all of this, maybe we can be more defensive and wrap it in a try/catch and fallback to 'ltr' - there are few external inputs here (the Jav locale) that might go wrong so it'd be good to make sure it doesn't crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible i18n.dir is already quite robust in that way -- you can pass it any string (or an array even, lol), and it will default to 'ltr'

Was there something else here you think is fragile?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I had locale being undefined in mind .. I thought it might be on first-render or something, and it's better to be safe. But maybe a try/catch around the whole useLocale effect is even safer (the stuff with splitting locale strings could throw as well in a couple of places)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my recent changes, I approached error handling in a bit more specific way at each step with useful fall-backs -- my idea is that if one step doesn't work, the other steps can still succeed and provide useful localization. Not many things in here can actually throw, but those things are wrapped in try/catch

const jsLocale = transformJavaLocale(locale)
const localeDirection = i18n.dir(jsLocale)
kabaros marked this conversation as resolved.
Show resolved Hide resolved

const globalDirection =
configDirection === 'auto' ? localeDirection : configDirection
// set `dir` globally (then override in app wrapper if needed)
document.documentElement.setAttribute('dir', globalDirection)

return localeDirection
}

export const useLocale = (locale) => {
const [result, setResult] = useState(undefined)
export const useLocale = ({ locale, configDirection }) => {
const [result, setResult] = useState({})

useEffect(() => {
if (!locale) {
return
}

const direction = handleDirection({ locale, configDirection })
setGlobalLocale(locale)
setResult(locale)
setResult({ locale, direction })
}, [locale, configDirection])

console.log('🗺 Global d2-i18n locale initialized:', locale)
}, [locale])
return result
}

Expand All @@ -47,16 +95,21 @@ 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({
locale:
data &&
(data.userSettings.keyUiLocale || window.navigator.language),
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 }
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 useDataQuery and i18n.dir, but I'll see what I can do 👍

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, just the useLocale hook will be quite a bit simpler -- this comment was placed in a different hook though 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests added

}
6 changes: 6 additions & 0 deletions cli/config/d2.config.app.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
const config = {
type: 'app',

// sets the `dir` HTML attribute for the app:
// options are 'ltr', 'rtl', and 'auto'. If set to 'auto', the direction
// will be inferred by the user's UI locale.
// The header bar direction will always be set by the locale.
direction: 'ltr',
KaiVandivier marked this conversation as resolved.
Show resolved Hide resolved

entryPoints: {
app: './src/App.js',
},
Expand Down
1 change: 1 addition & 0 deletions cli/src/lib/shell/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module.exports = ({ config, paths }) => {
const baseEnvVars = {
name: config.title,
version: config.version,
direction: config.direction,
}

return {
Expand Down
Loading
Loading