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

Language redirector #503

Closed
wants to merge 8 commits into from
Closed

Conversation

AHOHNMYC
Copy link
Contributor

@AHOHNMYC AHOHNMYC commented May 14, 2020

Copy link
Member

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

Yay, I really like this, thanks for the PR. I've left some comments, one other thing that could be changed is modifying the CONTRIBUTING.md file so that the translation step 1 points to the specific wiki page that we are now using as source for the languages.

assets/js/langRedirector.js Outdated Show resolved Hide resolved
assets/js/langRedirector.js Outdated Show resolved Hide resolved
assets/js/langRedirector.js Outdated Show resolved Hide resolved
assets/js/langRedirector.js Outdated Show resolved Hide resolved
@AHOHNMYC
Copy link
Contributor Author

There were too high question_per_line level, so I decided to rewrite the code 🐈

assets/js/langRedirector.js Outdated Show resolved Hide resolved
// Wrap all this to not pollute global namespace
{
// Generate an array of languages from page
const JDM_LANGS = [...document.querySelectorAll('#language-dropdown > ul > li')].map(el => el.classList[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Define this as a global constant via <script> block in Jekyll instead of parsing languages out of a query selector of language dropdown links.

Comment on lines +8 to +10
const link = document.querySelector(`#language-dropdown > ul > li.${lang} > a`);
if (link)
link.click();
Copy link
Member

Choose a reason for hiding this comment

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

Since our language URL is very simple, I'd rather set window.location to perform this redirection.

Suggested change
const link = document.querySelector(`#language-dropdown > ul > li.${lang} > a`);
if (link)
link.click();
if (lang) {
window.location = `${(new URL(window.location)).origin}/${lang}`;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not as simple. At least, we have en that points to /:

Suggested change
const link = document.querySelector(`#language-dropdown > ul > li.${lang} > a`);
if (link)
link.click();
if (lang) {
let path = lang;
if (lang === 'en')
path = '';
location = location.origin+'/'+path;
}

@AHOHNMYC
Copy link
Contributor Author

AHOHNMYC commented May 16, 2020

Btw, I've find great tool that seems to be more authoritative than Wikipedia: https://demo.icu-project.org/icu-bin/locexp#region
Chromium and Firefox uses ICU, so we have to use it like them :)
Link added in the first comment.

Instead of first test on all languages
Copy link
Member

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

Can you mark the issues that you've solved as such? I think there's one pending from @Nightfirecat and my new ones.

Also do note that you need to rebase your branch as it currently has conflicts with master

Comment on lines +46 to +47
localStorage.setItem('langRedirectionHasDone', true);
redirectTo(guessedTranslation());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
localStorage.setItem('langRedirectionHasDone', true);
redirectTo(guessedTranslation());
redirectTo(guessedTranslation());
localStorage.setItem('langRedirectionHasDone', true);

I guess I'd swap the order here, it seems more legit hahah

Comment on lines +14 to +41
function guessedTranslation() {
const langs = navigator.languages;

for (let i=0; i < langs.length; i++) {
let lang = langs[i];

// user has: 'pt-BR'
// we have: 'pt-BR'
if (JDM_LANGS.includes(lang))
return lang;

// user has: 'ru-RU'
// we have: 'ru'
lang = lang.slice(0,2);
if (JDM_LANGS.includes(lang))
return lang;

// user has: 'pt-AO' or 'pt'
// we have: 'pt-BR'
for (let j=0; j < JDM_LANGS.length; j++) {
let jdm_lang = JDM_LANGS[j];
if (jdm_lang.slice(0,2) === lang)
return jdm_lang;
}
}

return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function guessedTranslation() {
const langs = navigator.languages;
for (let i=0; i < langs.length; i++) {
let lang = langs[i];
// user has: 'pt-BR'
// we have: 'pt-BR'
if (JDM_LANGS.includes(lang))
return lang;
// user has: 'ru-RU'
// we have: 'ru'
lang = lang.slice(0,2);
if (JDM_LANGS.includes(lang))
return lang;
// user has: 'pt-AO' or 'pt'
// we have: 'pt-BR'
for (let j=0; j < JDM_LANGS.length; j++) {
let jdm_lang = JDM_LANGS[j];
if (jdm_lang.slice(0,2) === lang)
return jdm_lang;
}
}
return null;
}
function exactLangMatch(lang) {
// user has: 'pt-BR'
// we have: 'pt-BR'
return (JDM_LANGS.includes(lang) ? lang : null);
}
function partialLangMatch(lang) {
// user has: 'ru-RU'
// we have: 'ru'
lang = lang.slice(0,2);
return exactLangMatch(lang);
}
function roughLangMatch(lang) {
// user has: 'pt-AO' or 'pt'
// we have: 'pt-BR'
for (let j=0; j < JDM_LANGS.length; j++) {
let jdm_lang = JDM_LANGS[j];
if (jdm_lang.slice(0,2) === lang)
return jdm_lang;
}
return null;
}
function guessedTranslation() {
const langs = navigator.languages;
for (let i=0; i < langs.length; i++) {
let lang = langs[i];
let exactLang = exactLangMatch(lang);
if (exactLang !== null)
return exactLang;
let partialLang = partialLangMatch(lang);
if (partialLang !== null)
return partialLang;
let roughLang = roughLangMatch(lang.slice(0,2));
if (roughLang !== null)
return roughLang;
}
return null;
}

I'd suggest extracting each of the matches into a function. For the second one we can actually reuse the first and it moves the comments there. I don't particularly like all the if !== null, but I think overall it's clearer. Let me know what you guys think.

@tupaschoal
Copy link
Member

Another pending past issue:

one other thing that could be changed is modifying the CONTRIBUTING.md file so that the translation step 1 points to the specific wiki page that we are now using as source for the languages.

@tupaschoal
Copy link
Member

Hello @AHOHNMYC , do you intend to continue working on this?

No pressure, just want to know to see how we can move forward or help you :)

@tupaschoal
Copy link
Member

Hello @AHOHNMYC ,

I'm going to close this soon if you don't intend to continue, to clean up the PRs. Feel free to reopen if you want to resume it, it's a very nice addition!

Thanks

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.

Auto-redirect to localized version
3 participants