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

fix(ssr): use different language tag when with duplicate langs #7921

Merged
merged 10 commits into from
Feb 16, 2023

Conversation

yin1999
Copy link
Member

@yin1999 yin1999 commented Jan 7, 2023

Summary

See the result of hreflang tag testing tool on https://developer.mozilla.org/zh-CN/docs/Web/JavaScript/Reference/Global_Objects/Array:

image

Problem

  1. There are duplicate language tags when build zh-CN page and zh-TW is avaliable.
  2. The hreflang tag that used is lang-region is not prefered now, we may switch to lang-script format now. (as a part of <html> lang attribute "zh-CN", "zh-TW" does not conform BCP 47 Language Subtag Registry content#23596)

Solution

To resolve this, we should also include zh-CN locale when try to resolve language tag for zh-TW.


Screenshots

Before

See problem description.

After

See testing.


How did you test this change?

run yarn test:testing

@yin1999 yin1999 marked this pull request as ready for review January 7, 2023 03:15
@yin1999
Copy link
Member Author

yin1999 commented Jan 13, 2023

Marked as draft to fix mdn/content#23596 (comment)

@yin1999 yin1999 marked this pull request as ready for review January 15, 2023 07:12
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit.

ssr/render.ts Outdated
Comment on lines 20 to 23
const LANGUAGE_TAGS = new Map([
["zh-CN", "zh-Hans"],
["zh-TW", "zh-Hant"],
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a simple frozen object instead, and add the region?

Suggested change
const LANGUAGE_TAGS = new Map([
["zh-CN", "zh-Hans"],
["zh-TW", "zh-Hant"],
]);
const LANGUAGE_TAGS = Object.freeze({
'zh-CN': 'zh-Hans-CN',
'zh-TW': 'zh-Hant-TW',
});

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure should we just use zh-Hans for Simplified Chinese (zh-CN) and zh-Hant for traditional Chinese (zh-TW) without the region code (CN and TW). As we are using the shortest language tags that won't be duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, using zh-Hans and zh-Hant would be consistent with using pt for pt-BR.

ssr/render.ts Outdated Show resolved Hide resolved
@caugner caugner added the localization i18n & l10n label Feb 9, 2023
yin1999 and others added 2 commits February 10, 2023 08:09
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
ssr/render.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Tested locally (yarn dev and looked at the source of http://localhost:5042/en-US/docs/Learn/HTML), and works.

@caugner caugner merged commit 79fb3fc into mdn:main Feb 16, 2023
@yin1999 yin1999 deleted the fix-href branch February 16, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
localization i18n & l10n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants