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

Improved browser language (navigator.language) support #3904

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

iquidus
Copy link
Contributor

@iquidus iquidus commented Apr 19, 2020

589b174
On a users first visit (no cookie present) the language cookie was being set to padOptions.lang (from settings.json on server). Removes this behavior, allowing l10n to localize based on priority (cookie, navigator.language, navigator.userLanguage, 'en'), the users first visit will instead have no language cookie, giving browser language priority unless another language is selected in the menu (setting language cookie)

77a3059
Better handling of locale variants (e.g when browser language is ru-RU)
closes #3882

…ble locales

Before this change, we simply generated an error.

For example:
- if the browser sent 'ru-RU', but Etherpad has 'ru' available, select 'ru';
- if the browser sent 'zh', but we have 'zh-hans' available, use 'zh-hans'.

Fixes ether#3882.
@JohnMcLear
Copy link
Member

Needs a rebase :)

@iquidus
Copy link
Contributor Author

iquidus commented Apr 19, 2020

Travis CI failing with: SAUCE_USERNAME is unset - exiting. (env vars need to be set).

rebased

@JohnMcLear
Copy link
Member

Yea, we're aware of that, don't worry about it. It's a quirk with how we do automated testing.

@JohnMcLear
Copy link
Member

What happens here if the user changes their language and that is stored in the cookie? Is it respected?

@iquidus
Copy link
Contributor Author

iquidus commented Apr 19, 2020

yes, priority has not changed. Cookie language still takes priority over browser language, it's just not set to the server lang default when theres no cookie present, allowing browser language to apply.

@iquidus
Copy link
Contributor Author

iquidus commented Apr 19, 2020

This was already the case for the home page. However pads were getting overridden to server default as if there was no existing language cookie, one was being set, using lang from settings.json.

@JohnMcLear
Copy link
Member

+1 should be good to merge @iquidus did you run the front end tests btw? ''/tests/frontend'' in browser. Just wanna be sure we don't miss something obvious :D

@iquidus
Copy link
Contributor Author

iquidus commented Apr 19, 2020

passes: 96
failures: 0
duration: 255.47s
100%

@muxator muxator added the i18n label Apr 20, 2020
@muxator muxator added this to the 1.8.3 milestone Apr 20, 2020
@muxator
Copy link
Contributor

muxator commented Apr 20, 2020

This seems sufficiently small to pose no risk if integrated.

Scheduling for 1.8.3.

Comment on lines 184 to 193
if (lang.indexOf('-') > -1) {
var msg = 'Couldn\'t find translations for '+lang
lang = lang.split('-')[0] // get root locale incase variant (e.g ru-RU)
if (!data[lang]) { // check root locale (e.g ru)
var l
for(l in data) { // try similar (e.g zh-hans)
if(lang != l && l.indexOf(lang) === 0 && data[l]) {
lang = l
break;
}
}
if(lang != l) return cb(new Error(msg))
}
}
if(lang != l) return cb(new Error(msg))
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is clever.

Could you add a description in natural language of what this section is doing?
An example would be ok.

Imagine to have a nitwit in front of you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the var is a bit flinky in such deep scopes in js, but in this case we can live without let. Just saying.

@muxator
Copy link
Contributor

muxator commented Apr 21, 2020

I have pulled in the first commit (7ec3be6), on respecting browser's preferred language on first pad load).

I am waiting for the comments on the code before merging the second one (on language variants).

Thanks.

@muxator
Copy link
Contributor

muxator commented Apr 22, 2020

@iquidus: it would be good to merge your work on language variants for 1.8.3.

We have still some blockers in our milestone, so there is some time. If you are too busy, this last modification can be moved to the next release without problems.

Thanks.

@muxator
Copy link
Contributor

muxator commented Apr 24, 2020

Thanks @iquidus. I will merge this ASAP.

@muxator
Copy link
Contributor

muxator commented Apr 26, 2020

Merged, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Couldn't find translations for ru-RU
3 participants