You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:46:28 UTC
[GitHub] [incubator-echarts] wf123537200 opened a new pull request #13038: i18n rebuild
wf123537200 opened a new pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [ ] bug fixing
- [ ] new feature
- [x] others
### What does this PR do?
<!-- USE ONCE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
Refactor the i18n solution
### Fixed issues
#12926
## Details
### Before: What was the problem?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
i18n depends on packaging solution
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
### After: How is it fixed in this PR?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
as No.9 in the #12926 , refactor the i18n solution
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
## Usage
### Are there any API changes?
- [x] The API has been changed.
<!-- LIST THE API CHANGES HERE -->
add the method as
```
echarts.registerLocale('ES', lang);
```
add the params in the ```echarts.init``` as
```
echarts.init(document.getElementById('main3'), null, {
locale: 'ES'
});
```
### Related test cases or examples to use the new APIs
lang.html
## Others
### Merging options
- [x] Please squash the commits into a single one when merge.
### Other information
need to be done：
1、review the i18n solution is right
2、delete lang packaging code
3、check the test case is cover all the situation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038#issuecomment-665415541
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/incubator-echarts/wiki/How-to-make-a-pull-request).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang merged pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038#issuecomment-666446786
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] plainheart commented on a change in pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038#discussion_r462713963
##########
File path: src/component/toolbox/ToolboxModel.ts
##########
@@ -31,6 +31,7 @@ import {
CommonTooltipOption,
Dictionary
} from '../../util/types';
+import GlobalModel from "../../model/Global";
Review comment:
Note that we should use `'` but not `"`, check everywhere and correct them, please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038#discussion_r462026563
##########
File path: build/build-i18n.js
##########
@@ -0,0 +1,69 @@
+const fs = require('fs');
+
+const outFilePath = './i18n';
+const umdWrapperHead = `
+(function(root, factory) {
+ if (typeof define === 'function' && define.amd) {
+ // AMD. Register as an anonymous module.
+ define(['exports'], factory);
+ } else if (
+ typeof exports === 'object' &&
+ typeof exports.nodeName !== 'string'
+ ) {
+ // CommonJS
+ factory(exports);
+ } else {
+ // Browser globals
+ factory({});
+ }
+})(this, function(exports) {
+var lang =`;
Review comment:
Seems there is no export here.
Also obj export is not convenient in the case which loads script with HTML tag. I think we can keep it simple and only use CommonJS exports for the `-obj` files.
##########
File path: src/echarts.ts
##########
@@ -350,6 +356,13 @@ class ECharts extends Eventful {
this._theme = theme;
+ const {locale = 'ZH'} = opts;
+ // set default lang package
+ localeStorage['ZH'] = localeStorage['ZH'] || langZH;
+ localeStorage['EN'] = localeStorage['EN'] || langEN;
+ this._locale = typeof locale === 'string' ? localeStorage[locale] : zrUtil.clone(locale);
+ console.log(this._locale)
+
Review comment:
Needs to remove `console.log` here
##########
File path: src/model/Global.ts
##########
@@ -417,6 +422,25 @@ class GlobalModel extends Model<ECUnitOption> {
return this._theme;
}
+ getLocale(): LocaleOption {
+ return this._locale;
+ }
+
+ getWithLocale(localePosition: Array<string>, optionsPosition?: Array<string>, localeHandlerFn?: (text: string) => string): any {
+ // console.log(optionsPosition, localePosition);
+ const locale = this.getLocale()
+ let localeText: string | any;
+ localePosition.map(t => {
+ localeText = localeText ? localeText[t] : locale[t];
Review comment:
localePosition can also be wrapped with a Model to do deep query.
For example:
```
const localeModel = this.getLocaleModel();
const localeText = localeModel.get(localePosition);
```
##########
File path: src/echarts.ts
##########
@@ -92,6 +92,9 @@ import { getVisualFromData, getItemVisualFromData } from './visual/helper';
import LabelManager from './label/LabelManager';
import { deprecateLog } from './util/log';
import { handleLegacySelectEvents } from './legacy/dataSelectAction';
+// default import ZH and EN lang
+import langEN from "../i18n/langEN";
+import langZH from "../i18n/langZH";
Review comment:
I think it's better to put builtin `langEN` and `langZH` in the `src/i18n`. To keep all the source files in `src` folder. Or it may have unexpected issues when bundling. For examples, typescript compiler will check if the files are in the root `src` folder.
##########
File path: src/echarts.ts
##########
@@ -2459,6 +2475,11 @@ export function registerTheme(name: string, theme: ThemeOption): void {
themeStorage[name] = theme;
}
+export function registerLocale(name: string, locale: LocaleOption): void {
+ localeStorage[name] = locale;
+ console.log('localeStorage', localeStorage);
+}
Review comment:
Needs to remove console.log here
##########
File path: src/model/Global.ts
##########
@@ -417,6 +422,25 @@ class GlobalModel extends Model<ECUnitOption> {
return this._theme;
}
+ getLocale(): LocaleOption {
+ return this._locale;
+ }
+
+ getWithLocale(localePosition: Array<string>, optionsPosition?: Array<string>, localeHandlerFn?: (text: string) => string): any {
+ // console.log(optionsPosition, localePosition);
+ const locale = this.getLocale()
+ let localeText: string | any;
+ localePosition.map(t => {
+ localeText = localeText ? localeText[t] : locale[t];
+ })
+
+ if(localeHandlerFn) {
+ localeText = localeHandlerFn(localeText);
Review comment:
Extra space after '=' here.
Also, I'm not sure the last parameter `localeHandleFn` is necessary. It will increase the complexity of using this function.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038#discussion_r463073176
##########
File path: src/component/toolbox/ToolboxModel.ts
##########
@@ -31,6 +31,7 @@ import {
CommonTooltipOption,
Dictionary
} from '../../util/types';
+import GlobalModel from "../../model/Global";
Review comment:
@plainheart Thanks for the detailed review. I just fixed it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] wf123537200 commented on a change in pull request #13038: i18n rebuild
Posted by GitBox <gi...@apache.org>.
wf123537200 commented on a change in pull request #13038:
URL: https://github.com/apache/incubator-echarts/pull/13038#discussion_r462031394
##########
File path: build/build-i18n.js
##########
@@ -0,0 +1,69 @@
+const fs = require('fs');
+
+const outFilePath = './i18n';
+const umdWrapperHead = `
+(function(root, factory) {
+ if (typeof define === 'function' && define.amd) {
+ // AMD. Register as an anonymous module.
+ define(['exports'], factory);
+ } else if (
+ typeof exports === 'object' &&
+ typeof exports.nodeName !== 'string'
+ ) {
+ // CommonJS
+ factory(exports);
+ } else {
+ // Browser globals
+ factory({});
+ }
+})(this, function(exports) {
+var lang =`;
Review comment:
the exports code at the L54 like
```
##########
File path: build/build-i18n.js
##########
@@ -0,0 +1,69 @@
+const fs = require('fs');
+
+const outFilePath = './i18n';
+const umdWrapperHead = `
+(function(root, factory) {
+ if (typeof define === 'function' && define.amd) {
+ // AMD. Register as an anonymous module.
+ define(['exports'], factory);
+ } else if (
+ typeof exports === 'object' &&
+ typeof exports.nodeName !== 'string'
+ ) {
+ // CommonJS
+ factory(exports);
+ } else {
+ // Browser globals
+ factory({});
+ }
+})(this, function(exports) {
+var lang =`;
Review comment:
the exports code at the L54 like
```
const pureExports = `
exports.lang = lang;
`;
```
##########
File path: src/model/Global.ts
##########
@@ -417,6 +422,25 @@ class GlobalModel extends Model<ECUnitOption> {
return this._theme;
}
+ getLocale(): LocaleOption {
+ return this._locale;
+ }
+
+ getWithLocale(localePosition: Array<string>, optionsPosition?: Array<string>, localeHandlerFn?: (text: string) => string): any {
+ // console.log(optionsPosition, localePosition);
+ const locale = this.getLocale()
+ let localeText: string | any;
+ localePosition.map(t => {
+ localeText = localeText ? localeText[t] : locale[t];
+ })
+
+ if(localeHandlerFn) {
+ localeText = localeHandlerFn(localeText);
Review comment:
sometimes the text may handle after get，for example:
```
lang: ecModel.getWithLocale(['toolbox', 'saveAsImage', 'lang'], null, (t) => t.slice())
```
the ```toolebox.saveAsImage.lang``` is an array but need to be ```slice()``` after gotten.
##########
File path: src/model/Global.ts
##########
@@ -417,6 +422,25 @@ class GlobalModel extends Model<ECUnitOption> {
return this._theme;
}
+ getLocale(): LocaleOption {
+ return this._locale;
+ }
+
+ getWithLocale(localePosition: Array<string>, optionsPosition?: Array<string>, localeHandlerFn?: (text: string) => string): any {
+ // console.log(optionsPosition, localePosition);
+ const locale = this.getLocale()
+ let localeText: string | any;
+ localePosition.map(t => {
+ localeText = localeText ? localeText[t] : locale[t];
Review comment:
Sorry, I didn't quite understand what to do.
Should we add a method as ```getLocaleModel``` for user to get the model of locale?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org