[5.0] i18n rebuild #13038
[5.0] i18n rebuild #13038
Conversation
|
Thanks for your contribution! |
| @@ -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"; | |||
|
|
|||
There was a problem hiding this 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.
build/build-i18n.js
Outdated
| factory({}); | ||
| } | ||
| })(this, function(exports) { | ||
| var lang =`; |
There was a problem hiding this 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.
There was a problem hiding this comment.
the exports code at the L54 like
const pureExports = `
exports.lang = lang;
`;
| localeStorage['EN'] = localeStorage['EN'] || langEN; | ||
| this._locale = typeof locale === 'string' ? localeStorage[locale] : zrUtil.clone(locale); | ||
| console.log(this._locale) | ||
|
|
There was a problem hiding this comment.
Needs to remove console.log here
src/echarts.ts
Outdated
| export function registerLocale(name: string, locale: LocaleOption): void { | ||
| localeStorage[name] = locale; | ||
| console.log('localeStorage', localeStorage); | ||
| } |
There was a problem hiding this comment.
Needs to remove console.log here
src/model/Global.ts
Outdated
| const locale = this.getLocale() | ||
| let localeText: string | any; | ||
| localePosition.map(t => { | ||
| localeText = localeText ? localeText[t] : locale[t]; |
There was a problem hiding this comment.
localePosition can also be wrapped with a Model to do deep query.
For example:
const localeModel = this.getLocaleModel();
const localeText = localeModel.get(localePosition);
There was a problem hiding this 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?
src/model/Global.ts
Outdated
| }) | ||
|
|
||
| if(localeHandlerFn) { | ||
| localeText = localeHandlerFn(localeText); |
There was a problem hiding this 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.
There was a problem hiding this 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.
| @@ -31,6 +31,7 @@ import { | |||
| CommonTooltipOption, | |||
| Dictionary | |||
| } from '../../util/types'; | |||
| import GlobalModel from "../../model/Global"; | |||
There was a problem hiding this comment.
Note that we should use ' but not ", check everywhere and correct them, please.
There was a problem hiding this comment.
@plainheart Thanks for the detailed review. I just fixed it
|
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
Brief Information
This pull request is in the type of:
What does this PR do?
Refactor the i18n solution
Fixed issues
#12926
Details
Before: What was the problem?
i18n depends on packaging solution
After: How is it fixed in this PR?
as No.9 in the #12926 , refactor the i18n solution
Usage
Are there any API changes?
add the method as
add the params in the
echarts.initasRelated test cases or examples to use the new APIs
lang.html
Others
Merging options
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