-
Notifications
You must be signed in to change notification settings - Fork 509
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
automatically change language if cookied #3968
automatically change language if cookied #3968
Conversation
This is so cool Peter and works really well except for one use case 🙃. So, if I change my language to say Spanish and then navigate around, get to a page that looks to be out of date, and click the "View in English" link, it will just keep redirecting me back to Could we make it so that the "View in English" clears the cookie value or, somehow bypasses it? |
) { | ||
const newPathname = translateURL(pathname, locale, cookieLocale); | ||
// Just to be absolutely paranoidly certain it's not going to redirect | ||
// to the URL you're already don, we're doing this extra check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// to the URL you're already don, we're doing this extra check. | |
// to the URL you're already on, we're doing this extra check. |
We could definitely do that. Although I always get nervous about trying to inject a OR, we consider deleting that link. Seriously. I.e. that it always, and only, has "Change language" in the top bar (right of breadcrumbs). |
I'm changing this to a draft because I clearly forgot to address the fact that the "View in English" button broke. |
I reckon this is a worthwhile experiment. Having the "View in English" link was an experiment in itself to get a sense of how often people would use it. We now have some numbers and I reckon this is the login next step. Roll this out, have a look at the numbers again. If you see that now, there are a lot more events where people switch from X to English using the language selector, then perhaps that could be a signal that we need to bring back the link. I suspect that with this redirect logic, we are going to see people come into a document, change to their actually preferred language, and then be done with it until they are in a different browser or they clear cookies. |
I have three suggestions that keep the
Personally, I think the link will be useful because the authoritative source is always in English, and translations might not capture the meaning completely or be inaccurate or incomplete. |
@KartikSoneji Of all those wonderful suggestions, I would favor a simple '#localeOverride=en-US` or something. What we can do is something like this: <a href="#localOverride=en-US" onClick={event => {
changeLanguage('en-US');
}}>View in English</a> Then, it would change over to English instantly without a page load. And if they used right-click and "Open in new tab" the page would stay in English. You inspire me to give this a try! |
Of course! That is brilliantly simple. |
Now it works really well.
The screencast hopefully demonstrates all of this: https://user-images.githubusercontent.com/26739/125696581-79f-90d0-48ac-821a-3e3452f41981.mov @KartikSoneji Would you mind testing it out too. Do you know how to enable the translated content in a locale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#localeOverride
is working well, but it breaks all self-links on the English version of the page.
A simple fix is to remember if the locale was overridden even if the hash changes.
We can still retain the forward/back navigation with history.replaceState
and listening to the popstate
event. As an added bonus, it allows us to remove the hash after load, which I personally feel looks cleaner.
client/src/preferred-locale.ts
Outdated
let value = document.cookie | ||
.split("; ") | ||
.find((row) => row.startsWith(`${PREFERRED_LOCALE_COOKIE_NAME}=`)); | ||
if (value && value.includes("=")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but we can use the new optional chaining operator here (yay!).
if (value && value.includes("=")) { | |
if (value?.includes("=")) { |
|
||
React.useEffect(() => { | ||
const cookieValue = getPreferredCookieLocale(document); | ||
if (cookieValue && cookieValue.toLowerCase() !== "en-us") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (cookieValue && cookieValue.toLowerCase() !== "en-us") { | |
if (cookieValue?.toLowerCase() !== "en-us") { |
@@ -19,13 +21,36 @@ export function LanguageMenu({ | |||
native: string; | |||
}) { | |||
const ga = useGA(); | |||
const { pathname } = useLocation(); | |||
const { hash, pathname } = useLocation(); | |||
const navigate = useNavigate(); | |||
const [preferredLocale, setPreferredLocale] = React.useState(locale); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const [preventLocaleOverride, setPreventLocaleOverride] = | |
React.useState(false); | |
React.useEffect(() => { | |
window.addEventListener("popstate", (event) => { | |
if (event.state?.resetLocaleOverride) { | |
setPreventLocaleOverride(false); | |
} | |
}); | |
}, []); | |
React.useEffect(() => { | ||
const cookieLocale = getPreferredCookieLocale(document); | ||
if ( | ||
locale && | ||
cookieLocale && | ||
locale.toLowerCase() !== cookieLocale.toLowerCase() && | ||
// If the URL is something like `#localeOverride` we omit this | ||
// automatic "redirect" because the user has most likely clicked | ||
// a link that means that want to "peek" at a locale that is | ||
// different from what their cookie prefers. | ||
!hash.toLowerCase().includes(LOCALE_OVERRIDE_HASH.toLowerCase()) && | ||
translations | ||
.map((t) => t.locale.toLowerCase()) | ||
.includes(cookieLocale.toLowerCase()) | ||
) { | ||
const newPathname = translateURL(pathname, locale, cookieLocale); | ||
// Just to be absolutely paranoidly certain it's not going to redirect | ||
// to the URL you're already don, we're doing this extra check. | ||
if (newPathname !== pathname) { | ||
navigate(newPathname); | ||
} | ||
} | ||
}, [locale, hash, pathname, navigate, translations]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React.useEffect(() => { | |
const cookieLocale = getPreferredCookieLocale(document); | |
if ( | |
locale && | |
cookieLocale && | |
locale.toLowerCase() !== cookieLocale.toLowerCase() && | |
// If the URL is something like `#localeOverride` we omit this | |
// automatic "redirect" because the user has most likely clicked | |
// a link that means that want to "peek" at a locale that is | |
// different from what their cookie prefers. | |
!hash.toLowerCase().includes(LOCALE_OVERRIDE_HASH.toLowerCase()) && | |
translations | |
.map((t) => t.locale.toLowerCase()) | |
.includes(cookieLocale.toLowerCase()) | |
) { | |
const newPathname = translateURL(pathname, locale, cookieLocale); | |
// Just to be absolutely paranoidly certain it's not going to redirect | |
// to the URL you're already don, we're doing this extra check. | |
if (newPathname !== pathname) { | |
navigate(newPathname); | |
} | |
} | |
}, [locale, hash, pathname, navigate, translations]); | |
React.useEffect(() => { | |
const cookieLocale = getPreferredCookieLocale(document); | |
const hasLocaleOverrideHash = hash | |
.toLowerCase() | |
.includes(LOCALE_OVERRIDE_HASH.toLowerCase()); | |
if ( | |
!preventLocaleOverride && | |
cookieLocale && | |
locale?.toLowerCase() !== cookieLocale.toLowerCase() && | |
// If the URL is something like `#localeOverride` we omit this | |
// automatic "redirect" because the user has most likely clicked | |
// a link that means that want to "peek" at a locale that is | |
// different from what their cookie prefers. | |
!hasLocaleOverrideHash && | |
translations | |
.map((t) => t.locale.toLowerCase()) | |
.includes(cookieLocale.toLowerCase()) | |
) { | |
const newPathname = translateURL(pathname, locale, cookieLocale); | |
// Just to be absolutely paranoidly certain it's not going to redirect | |
// to the URL you're already don, we're doing this extra check. | |
if (newPathname !== pathname) { | |
navigate(newPathname); | |
} | |
} | |
// Remember that the locale was overriden even if the hash changes, | |
// otherwise it breaks self-links. | |
// Remove the hash so the url looks cleaner. | |
if (hasLocaleOverrideHash) { | |
setPreventLocaleOverride(true); | |
window.history.pushState( | |
{ resetLocaleOverride: true }, | |
"", | |
window.location.href.replace(/#.*$/, "") | |
); | |
} | |
}, [preventLocaleOverride, locale, hash, pathname, navigate, translations]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix the self-links, and preserve forward/back navigation.
@peterbe diff --git a/client/src/ui/molecules/language-menu/index.tsx b/client/src/ui/molecules/language-menu/index.tsx
index 4ee9078c..b1a48e96 100644
--- a/client/src/ui/molecules/language-menu/index.tsx
+++ b/client/src/ui/molecules/language-menu/index.tsx
@@ -24,21 +24,35 @@ export function LanguageMenu({
const { hash, pathname } = useLocation();
const navigate = useNavigate();
const [preferredLocale, setPreferredLocale] = React.useState(locale);
+ const [preventLocaleOverride, setPreventLocaleOverride] =
+ React.useState(false);
+
+ React.useEffect(() => {
+ window.addEventListener("popstate", (event) => {
+ if (event.state?.resetLocaleOverride) {
+ setPreventLocaleOverride(false);
+ }
+ });
+ }, []);
// This effect makes you automatically navigate to the locale your cookie
// prefers if the current page's locale isn't what you prefer and the
// locale you prefer is one of the valid translations.
React.useEffect(() => {
const cookieLocale = getPreferredCookieLocale(document);
+ const hasLocaleOverrideHash = hash
+ .toLowerCase()
+ .includes(LOCALE_OVERRIDE_HASH.toLowerCase());
+
if (
- locale &&
+ !preventLocaleOverride &&
cookieLocale &&
- locale.toLowerCase() !== cookieLocale.toLowerCase() &&
+ locale?.toLowerCase() !== cookieLocale.toLowerCase() &&
// If the URL is something like `#localeOverride` we omit this
// automatic "redirect" because the user has most likely clicked
// a link that means that want to "peek" at a locale that is
// different from what their cookie prefers.
- !hash.toLowerCase().includes(LOCALE_OVERRIDE_HASH.toLowerCase()) &&
+ !hasLocaleOverrideHash &&
translations
.map((t) => t.locale.toLowerCase())
.includes(cookieLocale.toLowerCase())
@@ -50,7 +64,19 @@ export function LanguageMenu({
navigate(newPathname);
}
}
- }, [locale, hash, pathname, navigate, translations]);
+
+ // Remember that the locale was overriden even if the hash changes,
+ // otherwise it breaks self-links.
+ // Remove the hash so the url looks cleaner.
+ if (hasLocaleOverrideHash) {
+ setPreventLocaleOverride(true);
+ window.history.pushState(
+ { resetLocaleOverride: true },
+ "",
+ window.location.href.replace(/#.*$/, "")
+ );
+ }
+ }, [preventLocaleOverride, locale, hash, pathname, navigate, translations]);
return (
<form |
@KartikSoneji Not a day goes by where I don't feel sad that I haven't had a chance to enjoy your feedback. This tab's still open and I haven't forgotten. Just got to put out fires and planned work first. |
Of course, I completely understand. |
Would it be possible to get back on this? It would be a great improvement to the new UI. |
This is definitely on our list of items to pick up again as soon as possible. Thank you for the nudge @NiedziolkaMichal |
This pull request has merge conflicts that must be resolved before it can be merged. |
Closing in favor of the new experimental Remember language feature. 🎉 |
Fixes #3949
This is something we've wanted to do for a long time. I've thought about it for a long time too. #3949 is just one of many similar issues filed. A lot of people change the locale, but keep stumbling into different locales which are different from what they prefer. For example, a user in Spain prefers to read in en-US because they're comfortable with that and/or they know it's more up-to-date. But a Google search in their country for "mdn array foreach" links to the
/es/docs/Web/...
URL. Or, someone preferszh-CN
but there's a blog post or tweet with an absolute URL foren-US
.According to the GA event we have on the "Change language" form (document footer), only ~15% use that where they don't have a
preferredlocale
cookie already. In the last 30 days, ~107,000 times people have used the "Change language" button. And ~97,000 times people have clicked on the "Switch to English". So roughly 200k times people have had to change the language. With this PR, it effectively does that language switch automatically for them.It would have been nice to do this on the server, with a 302 redirect but then we'd need to make the CDN cache hashing depend on this cookie and the worst thing is that in the Lambda@Edge code (where we'd do that redirect), it can't know if the translation is available (unless it's for redirecting to
en-US
which can always be assumed to exist now that slugs are never translated).