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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4913bbc
feat(theme-classic): allow specifying query string for detecting loca…
urish Nov 6, 2021
0667311
Merge branch 'main' into i18n-querystring
Josh-Cena Apr 8, 2022
58ef062
fix TS
Josh-Cena Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/docusaurus-theme-classic/src/theme/NavbarItem/LocaleDropdownNavbarItem/index.tsx
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ export default function LocaleDropdownNavbarItem({
...props
}: Props): JSX.Element {
const {
i18n: {currentLocale, locales, localeConfigs},
i18n: {currentLocale, locales, localeConfigs, queryString},
} = useDocusaurusContext();
const alternatePageUtils = useAlternatePageUtils();

Expand All @@ -34,6 +34,7 @@ export default function LocaleDropdownNavbarItem({
const to = `pathname://${alternatePageUtils.createUrl({
locale,
fullyQualified: false,
queryString,
})}`;
return {
isNavLink: true,
Expand Down
10 changes: 9 additions & 1 deletion packages/docusaurus-theme-common/src/utils/useAlternatePageUtils.ts
View file
Open in desktop
Expand Up @@ -15,9 +15,11 @@ export function useAlternatePageUtils(): {
createUrl: ({
locale,
fullyQualified,
queryString,
}: {
locale: string;
fullyQualified: boolean;
queryString?: string;
}) => string;
} {
const {
Expand All @@ -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

}: {
locale: string;
// For hreflang SEO headers, we need it to be fully qualified (full protocol/domain/path...)
// For locale dropdown, using a path is good enough
fullyQualified: boolean;
// Appends a query string parameter with the selected locale
queryString?: string;
}) {
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

: '';
return `${fullyQualified ? url : ''}${getLocalizedBaseUrl(
locale,
)}${pathnameSuffix}`;
)}${pathnameSuffix}${queryStringSuffix}`;
}

return {createUrl};
Expand Down
2 changes: 2 additions & 0 deletions packages/docusaurus-types/src/index.d.ts
View file
Open in desktop
Expand Up @@ -126,13 +126,15 @@ export type I18nConfig = {
defaultLocale: string;
locales: [string, ...string[]];
localeConfigs: Record<string, Partial<I18nLocaleConfig>>;
queryString?: string;
};

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

};

export interface DocusaurusContext {
Expand Down
1 change: 1 addition & 0 deletions packages/docusaurus/src/server/configValidation.ts
View file
Open in desktop
Expand Up @@ -104,6 +104,7 @@ const I18N_CONFIG_SCHEMA = Joi.object<I18nConfig>({
localeConfigs: Joi.object()
.pattern(/.*/, LocaleConfigSchema)
.default(DEFAULT_I18N_CONFIG.localeConfigs),
queryString: Joi.string().optional(),
})
.optional()
.default(DEFAULT_I18N_CONFIG);
Expand Down
1 change: 1 addition & 0 deletions packages/docusaurus/src/server/i18n.ts
View file
Open in desktop
Expand Up @@ -85,6 +85,7 @@ Note: Docusaurus only support running one locale at a time.`,
locales,
currentLocale,
localeConfigs,
queryString: i18nConfig.queryString,
};
}

Expand Down