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

enhance(l10n): localize "(en-US)" indicator #10996

Merged
merged 3 commits into from
May 2, 2024
Merged

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Apr 25, 2024

Summary

(MP-1091)

Problem

  1. Links in other locales that point to the English locale often have a trailing indicator "(en-US)", but this is rather technical and not ideal from a user experience point of view.
  2. Not all links to the English locale have this indicator.

Solution

  1. Replace the hardcoded "(en-US)" indicator by a localized one (e.g. "(angl.)" in French), via CSS.
  2. Use a CSS attribute selector targeting links that start with /en-US/ to catch all (?) occurrences.

(Note: I will add the hreflang attribute in a subsequent PR.)


Screenshots

Locale Before After
es image image
fr image image
ja image image
ko image image
pt-BR image image
ru image image
zh-CN image image
zh-TW image image

How did you test this change?

Ran yarn dev locally and looked at http://localhost:3000/en-US/docs/Web/API in different locales.

## Approvals

  • es
  • fr
  • ja
  • ko
  • pt-BR
  • ru
  • zh-CN
  • zh-TW

Links in other locales that point to the English locale often
have a suffix "(en-US)", but this is rather technical and not
ideal from a user experience point of view.

This PR replaces the suffix by a localized suffix, added via CSS.
@github-actions github-actions bot added flaw-system issues and feature requests related to the flaws system macros tracking issues related to kumascript macros labels Apr 25, 2024
@caugner caugner marked this pull request as ready for review April 25, 2024 14:20
@caugner caugner requested a review from a team as a code owner April 25, 2024 14:20
@caugner caugner requested review from a team, cw118, mfuji09, t7yang, lex111, wisedog, josielrocha and Jalkhov and removed request for a team April 25, 2024 14:22
@caugner
Copy link
Contributor Author

caugner commented Apr 25, 2024

@cw118 @mfuji09 @t7yang @lex111 @wisedog @josielrocha @Jalkhov Can you please review the localized suffix in your language (by approving or suggesting a change), and let me know what you think about this change (by giving the PR a 👍 or 👎)? Thank you. 🙌

PS: On a related note, are you all in our Discord in the #localization channel? 👀

@leon-win
Copy link
Member

OK for ru 👌
But it looks unusual))

@caugner
Copy link
Contributor Author

caugner commented Apr 25, 2024

But it looks unusual

@leon-win Is this because you're used to the (en-US) suffix, or because this is not usual in Russian? The idea behind the change is that not every user is necessarily familiar with ISO language codes, and having "(English)" in the locale instead ensures that every user understands that the link points to English content.

@leon-win
Copy link
Member

But it looks unusual

@leon-win Is this because you're used to the (en-US) suffix, or because this is not usual in Russian? The idea behind the change is that not every user is necessarily familiar with ISO language codes, and having "(English)" in the locale instead ensures that ever user understands that the link points to English content.

I think because of my subjective habit.
From the point of view of the Russian language, the new version is more correct 👍

Copy link
Member

@cw118 cw118 left a comment

Choose a reason for hiding this comment

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

This looks great, thanks very much @caugner! (Pinging @mdn/yari-content-fr for more opinions)

}

html[lang="es"] a.only-in-en-us:after {
content: "(inglés)";
Copy link
Member

Choose a reason for hiding this comment

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

@Graywolf9 what do you think on this one? I'd like to say the following:

Suggested change
content: "(inglés)";
content: "(Inglés)";

Do you agree? or do you think it is better without the uppercase?

Copy link
Member

Choose a reason for hiding this comment

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

I already checked it, according this resource it shouldn't use uppercase for the first letter. so this is good!

@caugner caugner changed the title enhance(l10n): localize "(en-US)" suffix enhance(l10n): localize "(en-US)" indicator Apr 25, 2024
Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

According to Chinese grammar (both zh-CN and zh-TW), we will use full-width brackets instead of half-width brackets (we have placed this as a note in zh-tw translation guide and zh-cn translation guide).


Another point I noticed is that the current font size of the Chinese note text does not seem to have enough contrast with the main content text's.

I've opened a discuession in discord, and @Josh-Cena pointed this problem. So I tried comparing three different CSS styles (aligned with baseline, using <percentage> font-size: 80%, and with the current CSS style). The result is shown below:

image

It seems that the one with font-size: 80% would be better than the default one (rightmost). @jasonren0403 has the same opinion as me.

client/src/document/index.scss Outdated Show resolved Hide resolved
client/src/document/index.scss Outdated Show resolved Hide resolved
Copy link

@wisedog wisedog left a comment

Choose a reason for hiding this comment

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

LGTM.

@mdn/yari-content-ko Please any suggestions if you have an idea.

Copy link
Member

@JuanVqz JuanVqz left a comment

Choose a reason for hiding this comment

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

For the ES team looks good! @Graywolf9

Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

Very elegant code, love the use of the attribute selector. Approved from an engineering perspective 👍

@caugner
Copy link
Contributor Author

caugner commented Apr 30, 2024

@mfuji09 (cc @mdn/yari-content-ja) @josielrocha (cc @mdn/yari-content-pt-br) Can you please take a look if these changes look good to you in ja and pt-BR? 🙏

Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

Approving this from l10n-zh team. Thank you @caugner

@caugner
Copy link
Contributor Author

caugner commented May 2, 2024

@mfuji09 (cc @mdn/yari-content-ja) @josielrocha (cc @mdn/yari-content-pt-br) Can you please take a look if these changes look good to you in ja and pt-BR? 🙏

Heads-up @mdn/yari-content-ja and @mdn/yari-content-pt-br that this change will land tomorrow, so if you can review the changes before, that would be great.

Copy link
Contributor

@mfuji09 mfuji09 left a comment

Choose a reason for hiding this comment

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

The indicator for ja looks good!
Sorry too late!

@caugner caugner merged commit be8852c into main May 2, 2024
16 checks passed
@caugner caugner deleted the localize-en-us-link-suffix branch May 2, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaw-system issues and feature requests related to the flaws system macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants