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

FIX Set current locale based on actual defined dictionaries #1523

Merged

Conversation

GuySartorelli
Copy link
Member

Previously, the current locale would always be null if a 2-character locale was defined in php with i18n::set_locale() (e.g. i18n::set_locale('nl');), because no dictionaries would have been loaded at the time detectLocale() was called.

Issue

@@ -28,6 +30,7 @@ class i18n {
*/
setLocale(locale) {
this.currentLocale = locale;
this.autoDetectLocale = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

If someone explicitly sets a locale, we don't want to override their decision whenever a new dictionary is added.

Comment on lines +97 to +100
// Re-set current locale in case the new dictionary provides a better match than the old locale.
if (this.autoDetectLocale) {
this.currentLocale = this.detectLocale();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Dictionaries are added by separate .js files. This is the most reliable way to ensure that the current locale is detected according to all dictionaries that will be loaded in, allowing for slow network speeds. i.e. at the time each dictionary is loaded in, the most-correct current locale will be set.

We have no way of knowing ahead of time if more dictionaries will be loaded, so we can't reasonably defer until all of them are loaded.

@@ -174,10 +182,9 @@ class i18n {
* Detect document language settings by looking at <meta> tags.
* If no match is found, returns this.defaultLocale.
*
* @todo get by <html lang=''> - needs modification of SSViewer
Copy link
Member Author

Choose a reason for hiding this comment

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

It's already doing this as the first step

Comment on lines -212 to +218
for (let compareLocale in i18n.lang) {
for (let compareLocale in this.lang) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The dictionaries are added to this instance of i18n, not as a static property on the class.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Test locally, works good

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