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

feat(theme-classic): allow specifying query string for detecting locale changes #5890

Closed
wants to merge 3 commits into from

Conversation

urish
Copy link

@urish urish commented Nov 6, 2021

Motivation

This pull requests adds a queryString parameter to the i18n section 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 the i18n section of the configuration file, and observing that the links in the languages dropdown have the query string appended:

image

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 6, 2021
@netlify
Copy link

netlify bot commented Nov 6, 2021

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 58ef062
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/624fe5750a7d45000804eaa6
😎 Deploy Preview https://deploy-preview-5890--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Nov 6, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 70
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5890--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Nov 7, 2021
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

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)}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@slorber
Copy link
Collaborator

slorber commented Nov 17, 2021

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

@urish
Copy link
Author

urish commented Nov 17, 2021

Thanks for following up! I'm currently busy working on my talk for Remoticon.2
, but should be available to look at the requested changes next week

@Josh-Cena
Copy link
Collaborator

@urish would you be able to pick it up any time soon? I'm frankly not very familiar with what's going on

@urish
Copy link
Author

urish commented Dec 12, 2021

@Josh-Cena are you waiting for the implementation?

@Josh-Cena
Copy link
Collaborator

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.

@urish
Copy link
Author

urish commented Dec 25, 2021

I'm sorry, but it doesn't seem like I'll get to that any time soon...

@Josh-Cena
Copy link
Collaborator

No worries! I'll see what we can do to push it forward

@urish urish requested a review from Josh-Cena as a code owner April 8, 2022 07:24
@urish urish closed this Oct 24, 2022
@wceolin
Copy link
Contributor

wceolin commented Apr 20, 2023

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 setCookie config but I saw @slorber's comment about not wanting to deal with cookies and this queryString proposal seems like a nice workaround.

@slorber
Copy link
Collaborator

slorber commented Apr 20, 2023

@wceolin I'm open to review your PR ;)

@wceolin
Copy link
Contributor

wceolin commented Apr 20, 2023

Thanks, @slorber. I'll work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants