feat: add configurable dir for language directions to app adapter [DHIS2-16480]
#825
Conversation
| @@ -12,6 +12,7 @@ const AppAdapter = ({ | |||
| appVersion, | |||
| url, | |||
| apiVersion, | |||
| direction, | |||
There was a problem hiding this comment.
do we need to set the default to 'ltr' here in case no value was provided?
There was a problem hiding this comment.
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.
yeah I would say so .. it would keep the default env vars tidier
adapter/src/utils/useLocale.js
Outdated
| 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`) | ||
| } | ||
|
|
There was a problem hiding this comment.
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}``
There was a problem hiding this comment.
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:
adapter/src/utils/useLocale.js
Outdated
| // (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 }) => { | ||
| // for i18n.dir, need JS-formatted locale |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
|
|
||
| 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.
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.
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 👍
There was a problem hiding this comment.
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.
Ah okay, just the useLocale hook will be quite a bit simpler -- this comment was placed in a different hook though 😁
| // proposed property | ||
| if (userSettings.keyUiLanguageTag) { | ||
| return new Intl.Locale(userSettings.keyUiLanguageTag) | ||
| } |
There was a problem hiding this comment.
This is the new property Austin and I proposed, as a proof of concept of forward and backward compatibilty
| * 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.
New fixes in here to Moment locale loading since the last review, and also split to its own function
| * 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.
Reworked the below take advantage of Intl.Locale semantics
# [10.5.0-alpha.1](v10.4.0...v10.5.0-alpha.1) (2024-01-25) ### Features * `dir` config for language directions and other i18n fixes [DHIS2-16480] ([#825](#825)) ([e605143](e605143))
|
🎉 This PR is included in version 10.5.0-alpha.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
…2-16480] (#825) * feat: change text direction based on locale * feat: add direction config to d2.config * refactor: split up handleLocale * refactor: rename function; add dir default to adapter * fix: remove direction from d2.config defaults in CLI * refactor: parse locale to Intl.Locale object * refactor: better i18nLocale logic * fix: moment locale formatting & add fallbacks * refactor: fn rename * refactor: move locale utils to a new file * test: set up test file for useLocale.test * fix: skip logic for en and en-US * test: add first useLocale tests * test: userSettings cases * test: add document direction tests * fix: handle nonstandard configDirections * feat: set document `lang` attribute
* feat: `dir` config for language directions and other i18n fixes [DHIS2-16480] (#825) * feat: change text direction based on locale * feat: add direction config to d2.config * refactor: split up handleLocale * refactor: rename function; add dir default to adapter * fix: remove direction from d2.config defaults in CLI * refactor: parse locale to Intl.Locale object * refactor: better i18nLocale logic * fix: moment locale formatting & add fallbacks * refactor: fn rename * refactor: move locale utils to a new file * test: set up test file for useLocale.test * fix: skip logic for en and en-US * test: add first useLocale tests * test: userSettings cases * test: add document direction tests * fix: handle nonstandard configDirections * feat: set document `lang` attribute * fix: remove unused export for useLocale (and update tests) (#826) * test: update tests to target useCurrentUserLocale * fix: remove unused export on useLocale --------- Co-authored-by: Kai Vandivier <[email protected]>
DHIS2-16480
This extends the CLI, App Shell, and App Adapter to set up the
dirHTML attribute to support RTL layouts.The "global"
dirat the top level of the document is set to the direction that's specified by the d2.config option so that it affects modals, alerts, and other portal elements, and then the user's locale direction is passed down to the header bar.This also includes some logic to improve the handling of locales: currently, specific locales like "Arabic (Egypt)" (
ar_EG) which are available from the back end aren't well supported -- for one thing, we should extend our efforts in Transifex to provide translations for those. In the App Adapter, however, we can check to see if translations exist for that specific locale, and if not, try to fall back to a more general locale likear. That behavior can be seen in the console of the RTL screenshots below.It also includes fixes for Moment, which doesn't accept locales in the format used before. I also added some fallback logic there to use a similar locale when possible
ar_EG with
direction: 'auto'(notice console anddirattributes in the DOM)ar_EG with default
direction(ltr)en
