Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
290 stars 76 forks source link

date-picker locale attr/prop value should be case insensitive #3925

Closed jcfranco closed 2 years ago

jcfranco commented 2 years ago

Actual Behavior

Stems from https://github.com/Esri/calcite-components/issues/3602#issuecomment-1011314264

When using locale="zh-CN" and locale="zh-cn", only the former works as it matches its localization file (the former defaults to English).

Note: this also applies to any supported locale with a language and region code

Expected Behavior

Using locale="zh-CN" or locale="zh-cn" should load the zh-CN.json localization file.

Reproduction Sample

https://codepen.io/jcfranco/pen/RwLqvme?editors=1000

Reproduction Steps

  1. Compare groups with different cased locale values.

Reproduction Version

@esri/calcite-components@1.0.0-beta.74

Relevant Info

Regression? Last working version: @esri/calcite-components@1.0.0-VERSION

Per https://tools.ietf.org/search/bcp47#section-2.1.1, the value of locale should be case-insensitive:

2.1.1. Formatting of Language Tags

At all times, language tags and their subtags, including private use and extensions, are to be treated as case insensitive: there exist conventions for the capitalization of some of the subtags, but these MUST NOT be taken to carry meaning.

Thus, the tag "mn-Cyrl-MN" is not distinct from "MN-cYRL-mn" or "mN- cYrL-Mn" (or any other combination), and each of these variations

Source

github-actions[bot] commented 2 years ago

More information is required to proceed with this issue:

This issue will be automatically closed in five days if the information is not provided. Thanks for your understanding.

AdelheidF commented 2 years ago

Is it expected that "zh" or "ZH" doesn't work but "pt" does work? Normally the language codes for those languages are more specific like "zh-ch" or "pt-br".

jcfranco commented 2 years ago

Looking at the implementation, it is expected. pt works because we have the fallback available. @raje9276 In this case, should we always provide the fallback alongside the specific locale values or just the latter?

FWIW, pt-PT.json is available, but pt-BR.json is missing.

github-actions[bot] commented 2 years ago

Installed and assigned for verification.

AdelheidF commented 2 years ago

FWIW, pt-PT.json is available, but pt-BR.json is missing.

We have a language code pt-BR. So what's the plan for that one? Currently it shows in English.

zh-cn, zh-hk, zh-tw work now. zh doesn't, but that seems to be expected.

jcfranco commented 2 years ago

We have a language code pt-BR. So what's the plan for that one? Currently it shows in English.

I think we should do 2 things:

  1. add support for pt-BR
  2. enhance the locale util to fetch the base language file if the specific one cannot be found and show English if this one fails.

I'll create issues for these.

jcfranco commented 2 years ago

Actually, the base language lookup worked before. We didn't have a test for this case, so this ended up introducing a regression. I'll create an issue for this. cc @y0n4

jcfranco commented 2 years ago

Regression issue created.

Would we still need to add the pt-BR locale file or would the existing pt one suffice? cc @raje9276

AdelheidF commented 2 years ago

Would we still need to add the pt-BR locale file or would the existing pt one suffice?

Olga says pt and pt-BR are the same for calendar.

raje9276 commented 2 years ago

@jcfranco pt is default for pt-PT (Portuguese Portugal) and it is different from pt-BR (Portuguese Brazilian). For Calendar the translations and formats are same but ESRI translation (resource files) are translated for pt-PT and pt-BR (different translations).

Did translation services team give you the translations?

jcfranco commented 2 years ago

@raje9276 Thanks for the info!

We have calendar resource files for pt.json and pt-PT.json, but none for pt-BR. If I understood correctly, this isn't an issue for our calendar use case.

Long term, in terms of maintainability would you suggest:

  1. Drop pt-PT.json and rely on pt.json?
  2. Add pt-BR.json even if it introduces another redundant file?
  3. Leave it as is since it would work regardless? (this seems a bit broken to me)

All of the above are from the context of calendar.

raje9276 commented 2 years ago

@jcfranco you need 2 files pt-PT.json (pt.json) and pt-BR.json even though they are the same.

We need 2 files because there is a possibility that the translations may change as we add more strings or the current translations change as part of language update.

jcfranco commented 2 years ago

Got it. I'll submit a PR to add the missing file. Thanks again!

benelan commented 2 years ago

verified on beta.76