-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-4581: Perform client-side redirect based on browser language #1094
Conversation
const location = useLocation(); | ||
|
||
const onSelectLocale = (locale) => { | ||
const localeHrefMap = getLocaleMapping(location.origin, slug); |
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.
Making this change since I realized we were constructing the whole mapping when we only want one path
super nit: when you say
Is the actual key to look for |
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.
LGTM. Just left a potential suggestion & comment.
src/hooks/use-locale-redirect.js
Outdated
return; | ||
} | ||
|
||
const { languages } = navigator; |
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.
Just curious, do you think useMemo
adds any benefits when grabbing the languages from the navigator
?
For example
const memorizedLanguages = useMemo(() => {
if (typeof navigator === 'undefined') {
return [];
}
return navigator.languages.map(lang => lang.split('-')[0]);
}, []);
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.
I'm not sure how much of a benefit we'd get memoizing the list of languages alone, but this did make me realize that the current useEffect
hook could potentially result in checking the user's list of languages multiple times while they're going through the docs with a non-supported browser language set (German, for example). Instead, I've opted to memoize the search for the user's preferred non-English language so we only do it once per user visit, instead of once per page.
I've refactored the code and updated the staging links accordingly. Can you let me know what you think? @caesarbell If it seems like the previous iteration made more sense and seemed more performant, I can go ahead and revert.
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.
some minor comments below! we had discussed a script tag to prevent flashes of one language before the redirect occurs, but we can take that in a different ticket if need be
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.
Overall, it looks good to me, I did leave a small question to @seungpark comment.
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 LGTM 🙌 thanks for including the unit test and the exported fn as string 🧠
} | ||
|
||
// Check to see if browser's language is one that is already translated | ||
const matchedLocale = supportedLocaleCodes.find((localeCode) => localeCode.includes(lang)); |
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.
minor nitpick. would prefer localeCode.startsWith vs includes in case of match on the second part of locale code, if possible(?)
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.
Good point. Done
type="text/javascript" | ||
dangerouslySetInnerHTML={{ | ||
// Call function immediately on load | ||
__html: `!${redirectBasedOnLang}()`, |
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.
nice!
Stories/Links:
DOP-4581
Current Behavior:
Prod
Staging Links:
English docs site example
Portuguese docs site example
Notes:
window.href
after the page loads based on the user's browser language settings. This does have the limitation of needing to wait for the JS to load before being sent to the localized page. Ideally, this would be logic on the server side, but since something like that would require changing our existing hosting infrastructure, we opted for client-side for now (see ticket for more information). Note that no 300 response is provided.head
tag of the HTML so that it can run almost immediately on page load, rather than needing to wait for React hydration if it were used as a typical hook within a component. This does have the caveat of potentially having duplicate code since the script itself cannot use imports due to how Webpack bundles things.To test:
Feel free to remove the new
preferredLocale
key from themongodb-docs
object in your browser's local storage if you need to reset settings.Open questions
README updates