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 dayjs i18n #755

Merged
merged 9 commits into from
Sep 21, 2020
Merged

fix dayjs i18n #755

merged 9 commits into from
Sep 21, 2020

Conversation

baurine
Copy link
Collaborator

@baurine baurine commented Sep 8, 2020

resolve #754

This bug should be brought by PR #677 which changed all 'zh-CN' to 'zh' limit the available languages to 'zh' and 'en', but dayjs doesn't have a 'zh' locale name, it should be 'zh-cn'.

Effect:

image

@@ -8,7 +8,11 @@ import { initReactI18next } from 'react-i18next'

i18next.on('languageChanged', function (lng) {
console.log('Language', lng)
dayjs.locale(lng.toLowerCase())
if (lng.startsWith('zh')) {
Copy link
Member

@breezewish breezewish Sep 14, 2020

Choose a reason for hiding this comment

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

The whole logic becomes.. zh-CN -> zh -> zh-CN now. Is it better to revert some the changes in #677? IMO it is better to leave some capability to support both zhCN and zhTW. Will it work if we fallback zh to zh-CN?

https://www.i18next.com/principles/fallback#fallback-language

Copy link
Collaborator Author

@baurine baurine Sep 16, 2020

Choose a reason for hiding this comment

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

Tried the fallback-language setting, it's a bit troublesome, you need to cover many languages, for example:

fallbackLng: { 
    'zh-HK': ['zh-CN', 'en'], 
    'zh-TW': ['zh-CN', 'en'],
    'zh-SG': ...,
    'zh-Hans': ...,
    'zh-Hant': ...,
    ...
}

According to some documents, the languages are also maybe:

zh-Hans 简体中文
zh-Hans-CN 大陆地区使用的简体中文
zh-Hans-HK 香港地区使用的简体中文
zh-Hans-MO 澳门使用的简体中文
zh-Hans-SG 新加坡使用的简体中文
zh-Hans-TW 台湾使用的简体中文
zh-Hant 繁体中文
zh-Hant-CN 大陆地区使用的繁体中文
zh-Hant-HK 香港地区使用的繁体中文
zh-Hant-MO 澳门使用的繁体中文
zh-Hant-SG 新加坡使用的繁体中文
zh-Hant-TW 台湾使用的繁体中文

(Although I don't meet)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think maybe we can fix this bug first. Then later if someone contributes the traditional Chinese translation for us, we can add the support for dayjs.

Copy link
Member

Choose a reason for hiding this comment

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

This fallback looks to be stupid. Why can't we just fallback zh to zh-CN?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure whether zh -> zh-CN will make zh-TW fallback to zh-CN, let me have a try.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried to revert it and use the following setting for i18next:

export const ALL_LANGUAGES = {
  'zh-CN': '简体中文',
  en: 'English',
}

i18next
  .use(LanguageDetector)
  .use(initReactI18next)
  .init({
    resources: {}, // oh! this line is a big pitfall, we can't remove it, else it will cause strange crash!
    fallbackLng: {
      zh: ['zh-CN'],
      default: ['en'],
    },
    interpolation: {
      escapeValue: false,
    },
  })

It works in most cases, except when you run the dashboard for the first time with another language likes zh-TW.

企业微信20200917112000

image

Because we don't import zh-tw locale for dayjs, and i18next's fallback doesn't affect dayjs.

But it will always works fine after manually selecting the 'zh-CN' or 'en' language.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also tried this:

import en from 'dayjs/locale/en'
import zh from 'dayjs/locale/zh-cn'

//...

const DAYJS_LOCALES = { zh, en }
// console.log(DAYJS_LOCALES)
i18next.on('languageChanged', function (lng) {
  dayjs.locale(DAYJS_LOCALES[lng.toLocaleLowerCase()] || en)
})

It works when language is zh, but crashes when en, I think the dayjs has bug (will check it later).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to find a good way:

import zh from 'dayjs/locale/zh-cn'

const DAYJS_LOCALES = { zh }

i18next.on('languageChanged', function (lng) {
  dayjs.locale(DAYJS_LOCALES[lng.toLocaleLowerCase()] || 'en')
})

Copy link
Member

@breezewish breezewish Sep 17, 2020

Choose a reason for hiding this comment

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

Maybe the real problem that #677 need to solve is that, the initial language is the detected language, instead of the effective language. So that by assigning the detected language it may not be a useful language.

Copy link
Collaborator Author

@baurine baurine Sep 17, 2020

Choose a reason for hiding this comment

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

So you mean we need write a util method getEffectiveLang() and use its value as Select component's default value, right?

I tried it and works fine. We don't need to revert the zh.yml to zh-CN.yml, and we shouldn't, else 'zh-TW' will fallback to 'en' translation instead of 'zh' translation.

export function getEffetiveLang(): string {
  const effetiveLangs = Object.keys(ALL_LANGUAGES)
  const detectedLang = i18next.language
  if (effetiveLangs.includes(detectedLang)) {
    return detectedLang
  }
  if (detectedLang.startsWith('zh')) {
    return 'zh-CN'
  }
  return 'en'
}

        <Form layout="vertical" initialValues={{ language: getEffetiveLang() }}>
          <Form.Item name="language" label={t('user_profile.i18n.language')}>
            <Select onChange={handleLanguageChange} style={{ width: 200 }}>
              {_.map(ALL_LANGUAGES, (name, key) => {
                return (
                  <Select.Option key={key} value={key}>
                    {name}
                  </Select.Option>
                )
              })}
            </Select>
          </Form.Item>
        </Form>

@@ -37,18 +35,32 @@ export function addTranslationResource(lang, translations) {
}

export const ALL_LANGUAGES = {
zh: '简体中文',
'zh-CN': '简体中文',
Copy link
Member

Choose a reason for hiding this comment

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

this might cause bugs since all locale files are now named as zh instead of zh-CN.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But actually it works fine after trying. Because all languages that start with 'zh-' will fallback to use 'zh' translation first if they can't find its own language translation, and this is i18next's default behavior. https://www.i18next.com/principles/fallback#locals-resolving

Comment on lines 42 to 53
export function getEffetiveLang(): string {
const effetiveLangs = Object.keys(ALL_LANGUAGES)
const detectedLang = i18next.language
if (effetiveLangs.includes(detectedLang)) {
return detectedLang
}
if (detectedLang.startsWith('zh')) {
return 'zh-CN'
}
return 'en'
}

Copy link
Member

Choose a reason for hiding this comment

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

Is this helpful? i18next/i18next#489 I found others may meet similar problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😂 Sorry, I didn't get the relationship between this method and issue i18next/i18next#489 .

@baurine
Copy link
Collaborator Author

baurine commented Sep 17, 2020

Hi @breeswish , the root cause of this bug is that the PR #677 limit the available languages to 'zh' and 'en' by whitelist: ['zh', 'en'] option, but for dayjs, we only import the 'en' and 'zh-cn' locales for it, we don't import 'zh' locale for it.

So I just found a new and simpler solution which works fine after trying. Just need to change one line code, to import 'zh' locale instead of 'zh-cn' locale for dayjs. (I checked 'zh' locale and 'zh-cn' locale are exactly same except the name).

- import 'dayjs/locale/zh-cn'
+ import 'dayjs/locale/zh'

How about this?

I don't think currently we need to support 'zh-TW' etc languages for dayjs. Because if we want to support this, we need to import an extra 'dayjs/locale/zh-tw' for it, but we don't prepare any other zh-TW.yml, it makes no much meaning.

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Neat!

@breezewish breezewish merged commit 18d3266 into pingcap:master Sep 21, 2020
@baurine baurine deleted the fix-dayjs-i18n branch September 21, 2020 12:38
breezewish pushed a commit that referenced this pull request Nov 26, 2020
breezewish added a commit that referenced this pull request Nov 26, 2020
* misc: Increase ulimit to 65535 for test env (#756)
* test: Fix frontend CI (#752)
* ui: fix dayjs i18n (#755)
* ui: handle error globally (#757)
* statement, slow_query: support all fields in list page (#749)
* ui: memorize expand/collapse full text in detail pages (#775)
* ui: break loop dependencies (#771)
* ui: fix browser compatibility check (#776)
* ui: Refine store location, add zoom and pan (#772)
* ui: show disk usage information for statement and slow query (#777)
* ui: use qps instead of ops (#786)
* statement: support export (#778)
*: Fix slow query and start_ts not working in some cases (#793)
* ui: fix errors doesn't display (#794)
* ui: fix the error message doesn't show correct (#799)
* slow_queries: support export (#792)
* ui: add MySqlFormatter to customize the sql formatter (#805)
*: fix query statement detail error cause by round (#806)
* ui: copy original content instead of formatted content for CopyLink (#802)
* add min height of topology canvas (#804)
* metrics: Support customize Prometheus address (#808)
* clusterinfo: Refine (#815)
* ui: Open statement and slow log in new tab (#816)
* ui: add more time field for slow query detail page (#810)
* slowlog: Improve descriptions (#817)
* build: add action to check release-version is changed for release branch
* Release v2020.11.26.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateTime component show incorrect i18n wording for Chinese
3 participants