feat(theme-classic): allow specifying query string for detecting locale changes #5890
Conversation
…le changes see facebook#5839 for more information
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5890--docusaurus-2.netlify.app/ |
There was a problem hiding this comment.
Thanks
This is not exactly what I suggested in #5839 (reply in thread) 😅 For now please do this only in the theme navbar locale dropdown component, not in core config.
In all cases we don't want that querystring to appear everywhere we construct alternate lang urls.
Also worth making the API more intuitive and write some doc
| }) { | ||
| const queryStringSuffix = queryString | ||
| ? `?${queryString}=${encodeURIComponent(locale)}` |
There was a problem hiding this comment.
Why do you need ?foo=pt-BR, can't ?persistLocale=true be good enough?
queryString config implicitly means you are passing a whole querystring as config (x=y), not a querystring attribute name (x).
We'd rather rename this queryString config to something more meaningful (like localeQueryStringParam: 'locale' ?), or pass the whole querystring object as is without unexpected extra logic
| }; | ||
|
|
||
| export type I18n = { | ||
| defaultLocale: string; | ||
| locales: [string, ...string[]]; | ||
| currentLocale: string; | ||
| localeConfigs: Record<string, I18nLocaleConfig>; | ||
| queryString?: string; |
There was a problem hiding this comment.
I don't think i18n is a good place to add this.
Persisting a chosen locale in some storage is a theming concern, not a core concern. It's not all or nothing, and locale change links shouldn't necessarily have this querystring.
plus a i18n.queryString config is not very intuitive to make it global as is
I'd rather have this as a locale dropdown config for now
| @@ -43,15 +45,21 @@ export function useAlternatePageUtils(): { | |||
| function createUrl({ | |||
| locale, | |||
| fullyQualified, | |||
| queryString, | |||
There was a problem hiding this comment.
This is not a good place to handle that
And this code is used to generate meta SEO hreflang headers too, which should not include that querystring
https://developers.google.com/search/docs/advanced/crawling/localized-versions
<link data-react-helmet="true" rel="canonical" href="https://docusaurus.io/">
<link data-react-helmet="true" rel="alternate" href="https://docusaurus.io/" hreflang="en">
<link data-react-helmet="true" rel="alternate" href="https://docusaurus.io/fr/" hreflang="fr">
<link data-react-helmet="true" rel="alternate" href="https://docusaurus.io/pt-BR/" hreflang="pt-BR">
<link data-react-helmet="true" rel="alternate" href="https://docusaurus.io/ko/" hreflang="ko">
<link data-react-helmet="true" rel="alternate" href="https://docusaurus.io/zh-CN/" hreflang="zh-CN">
<link data-react-helmet="true" rel="alternate" href="https://docusaurus.io/" hreflang="x-default">And a fully qualified URL (provided as param) may already contain a querystring
|
Let me know if you want help I think this feature is a nice idea and we should dogfood it on our own website. We use Netlify, not sure serverless functions are enough but maybe I can try to set up edge handlers |
|
Thanks for following up! I'm currently busy working on my talk for Remoticon.2 |
|
@urish would you be able to pick it up any time soon? I'm frankly not very familiar with what's going on |
|
@Josh-Cena are you waiting for the implementation? |
|
Yeah, I'm looking forward to your updated implementation following the review... If I had more context of what this PR is trying to achieve I may be able to help you :D But for now I prefer to sit back and let you handle this. |
|
I'm sorry, but it doesn't seem like I'll get to that any time soon... |
|
No worries! I'll see what we can do to push it forward |
|
I'm interested in this feature. I saw the PR was closed. Can I open a new one to address the comments on the review? What I'm trying to accomplish here is to figure out from Cloudflare Functions if a user is explicitly trying to change the locale. I currently have a workaround using an event listener to set cookies and I initially thought we have a |
|
@wceolin I'm open to review your PR ;) |
|
Thanks, @slorber. I'll work on it. |
Motivation
This pull requests adds a
queryStringparameter to thei18nsection of the configuration. Using this parameter allows external server-side i18n handling logic to be aware of the user's explicit locale changes, so it won't override the chosen locale with the one detected by looking at the browser's Accept header. See #5839 for more information.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
I manually tested this by adding
queryString: 'foobar'to thei18nsection of the configuration file, and observing that the links in the languages dropdown have the query string appended: