Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

added: Add Chinese regional text display content #47

Merged
merged 2 commits into from
Dec 23, 2023

Conversation

huifer
Copy link
Contributor

@huifer huifer commented Jul 11, 2023

Thank you for submitting a pull request!

Description

I've added Chinese regional text information to this project

Type of Pull Request

  • Adding a feature
  • Fixing a bug
  • Maintaining documents
  • Others ()

Verify the followings

  • Code is up-to-date with the main branch
  • No lint errors after npm run lint
  • Make sure all the exsiting features working well

Refer to CONTRIBUTING.MD for more details.

@huifer
Copy link
Contributor Author

huifer commented Jul 11, 2023

@JinIgarashi Please help assist to see if the relevant content is reasonable

Copy link
Member

@JinIgarashi JinIgarashi left a comment

Choose a reason for hiding this comment

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

@huifer Thank you for the pull request! This translation for Chinese languages is awesome! I just commented few minor things which you can change it before merging to master branch.

README.md Outdated
Comment on lines 124 to 125
- `zh_Hans_CN` Chinese Simplified
- `zh_Hant_CN` Chinese Traditional
Copy link
Member

Choose a reason for hiding this comment

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

Can we use - for language code instead of _? I think - is generally used to express a language code.

Also, do we need to add CN in the last of code? If there are any differences in translations by regions like China, Hong Kong , Taiwan..., maybe CN is necessary. But we may not need it in this plugin. I think shorter code might be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -16,7 +16,7 @@ type Options = {
Crosshair?: boolean;
PrintableArea: boolean;
accessToken?: string;
Local?: 'de' | 'en' | 'fr' | 'fi' | 'sv' | 'vi';
Local?: 'de' | 'en' | 'fr' | 'fi' | 'sv' | 'vi' | 'zh_Hans_CN' | 'zh_Hant_CN';
Copy link
Member

Choose a reason for hiding this comment

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

As I commented previously, we can use - and we may not need regional name like CN in a language code.

case 'zh_Hans_CN':
return zhHansCN;
case 'zh_Hant_CN':
return zhHantCN;
Copy link
Member

Choose a reason for hiding this comment

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

This one, too. See my previous comment

@@ -4,6 +4,8 @@ import finnish from './fi';
import german from './de';
import swedish from './sv';
import vietnam from './vi';
import zhHansCN from './zh_Hans_CN';
import zhHantCN from './zh_Hant_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 one, too. See my previous comment

@JinIgarashi
Copy link
Member

@huifer I am happy to merge this PR to master branch if you can rename files and variables as follows.

  • for the language parameter
  • zh_Hans_CN > zh-Hans
  • zh_Hant_CN > zh-Hant
  • for the variable name
    • zhHansCN > zhHans
    • zhHantCN > zhHant
  • for the local language files' name
    • zh_Hans_CN.ts > zhHans.ts
    • zh_Hant_CN.ts > zhHant.ts

Thanks

@huifer
Copy link
Contributor Author

huifer commented Dec 6, 2023

Let's confirm some final issues now. How would you suggest changing the name for the following content

zh_Hans_CN -> zh-Hans
zh_Hant_TW -> zh-Hant-TW
zh_Hant_HK -> zh-Hant-HK

@huifer
Copy link
Contributor Author

huifer commented Dec 6, 2023

@JinIgarashi Please help assist to see if the relevant content is reasonable

@JinIgarashi JinIgarashi merged commit 69b9993 into watergis:master Dec 23, 2023
1 check passed
@JinIgarashi
Copy link
Member

@huifer Thanks for changing. looks great

@JinIgarashi
Copy link
Member

@huifer v2.0.2 was now released. Thank you for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants