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

UI: Add a whitelist in i18next to fix the issue #675 #677

Merged
merged 6 commits into from
Jul 13, 2020

Conversation

ericsyh
Copy link
Contributor

@ericsyh ericsyh commented Jul 9, 2020

Why for change

To fix #675.
The root cause of issue is we declare two languages as zh-CN and en in i18next while some situations browser will return zh (Under my case the browser is Chrome 83 the OS is Windows 10) which is out of our language definition.

What did i do

I added a whitelist only for zh-CN and en in i18next to block other cases. With the whitelist i18next will fallback to en by default if browser return language out of our definition like zh or en-US.

Test

Passed on my environment. Now i18next got en as i18nextLng

image

@ericsyh
Copy link
Contributor Author

ericsyh commented Jul 9, 2020

@baurine PTAL :)

@baurine
Copy link
Collaborator

baurine commented Jul 9, 2020

So by this way, 'zh-HK' / 'zh-TW' / 'zh-SG' (https://www.andiamo.co.uk/resources/iso-language-codes/) will be fallback to 'en' as well (need extra tests to confirm), not sure whether i18next has a customized fallback strategy.

@ericsyh
Copy link
Contributor Author

ericsyh commented Jul 9, 2020

Seems no customized fallback provided in i18next but has custom Detector. I tried it but do not figure it out :(

@baurine
Copy link
Collaborator

baurine commented Jul 9, 2020

According to the document: https://www.i18next.com/principles/fallback

  1. Per default locals containing region or script will take a translation from the pure language file if not found. (https://www.i18next.com/principles/fallback#locals-resolving). It means 'zh-XX' will fallback to 'zh' automatically

  2. We can define an object for fallbackLng (https://www.i18next.com/principles/fallback#fallback-language) like this:

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

I prefer to use the way 1. We can change all places used by 'zh-CN' to 'zh'. (for example, zh-CN.yaml to zh.yaml), this can prevent 'zh' / 'zh-HK' / 'zh-TW' / 'zh-SG' / 'zh-XXX' except 'zh-CN' fallback to 'en' with no more configuration. (need test to confirm)

@breeswish , how do you think about it?

@baurine
Copy link
Collaborator

baurine commented Jul 9, 2020

Or I can try the custom detector later.

@breezewish
Copy link
Member

Yes, I also think solution 1 looks best

#677 (comment)

@baurine
Copy link
Collaborator

baurine commented Jul 10, 2020

Thanks for your contribution! LGTM, I will have a test soon.

Copy link
Collaborator

@baurine baurine left a comment

Choose a reason for hiding this comment

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

Works fine as expected.

@ti-srebot
Copy link

@baurine,Thanks for your review.

@baurine
Copy link
Collaborator

baurine commented Jul 10, 2020

/merge

@ti-srebot
Copy link

Sorry @baurine, you don't have permission to trigger auto merge event on this branch. The number of LGTM for this PR is 1 while it needs 2 at least

@@ -0,0 +1,5 @@
dashboard_settings:
Copy link
Member

Choose a reason for hiding this comment

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

We no longer have this file, you can remove it

Copy link
Contributor Author

@ericsyh ericsyh Jul 13, 2020

Choose a reason for hiding this comment

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

I see, you moved dashboard setting to the user profile page in previous commit. I will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Push a new commit 436817d to remove the dashboard setting page.

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.

The rest LGTM, thanks!

@breezewish
Copy link
Member

/merge

@ti-srebot
Copy link

/run-all-tests

@breezewish breezewish merged commit df16fb0 into pingcap:master Jul 13, 2020
@breezewish
Copy link
Member

@all-contributors please add @ericsyh for code

@allcontributors
Copy link
Contributor

@breeswish

I've put up a pull request to add @ericsyh! 🎉

@ericsyh ericsyh deleted the ericsyh/add-i18next-whitelist branch July 13, 2020 03:56
breezewish added a commit that referenced this pull request Jul 15, 2020
* ui: Fix TiFlash log searching (#680)
* ui: Add a whitelist in i18next to fix the issue #675 (#677)
* docs: add ericsyh as a contributor (#681)
* ui: refine code and error handle (#662)
* *: Rename disable_telemetry to enable_telemetry (#684)
* ui: Update space behaviour in statement / slow log search (#682)
* ui: No need to generate ui library (#687)
* forwarder: evict invalid current picked remote (#689)
* keyviz: support to merge cold logical range (#674)
* Update release version
@baurine baurine mentioned this pull request Sep 8, 2020
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.

i18n: the default language value is incorrect
4 participants