100pah commented on code in PR #20705:
URL: https://github.com/apache/echarts/pull/20705#discussion_r1924009599
##########
src/core/echarts.ts:
##########
@@ -693,10 +685,30 @@ class ECharts extends Eventful<ECEventDefinition> {
}
/**
- * @deprecated
+ * Update theme with name or theme option and repaint the chart.
+ * @param theme Theme name or theme option.
*/
- private setTheme(): void {
- deprecateLog('ECharts#setTheme() is DEPRECATED in ECharts 3.0');
+ setTheme(theme: string | ThemeOption): void {
+ this._updateTheme(theme);
+ if (this._model) {
+ this._model.setTheme(this._theme);
+ // Force refresh to apply theme changes
+ updateMethods.prepareAndUpdate.call(this, {
+ type: 'themeChanged'
Review Comment:
I think the `type: 'themeChanged'` is ineffective. It won't tigger an event.
it is probably inappropriate to trigger an event in an explicit API call (which
might lead to dead loop).
If needing to call `updateMethods.prepareAndUpdate` or
`updateMethods.update`, it's OK to use `null` as the 2nd parameter.
##########
src/model/Global.ts:
##########
@@ -543,6 +543,14 @@ echarts.use([${seriesImportName}]);`);
return option;
}
+ setTheme(theme: object) {
+ this._theme = new Model(theme);
+ // Merge theme with current option directly
+ mergeTheme(this.option, this._theme.option);
Review Comment:
The next sentence `this._resetOption('recreate', null);` will recreate
`ecModel.option`, which makes this `mergeTheme` ineffective.
I think it's OK to just remove this `mergeTheme`.
And the next sentence will call `initBase` to recreate a `ecModel.option`
and merge theme to it.
##########
src/core/echarts.ts:
##########
@@ -693,10 +685,30 @@ class ECharts extends Eventful<ECEventDefinition> {
}
/**
- * @deprecated
+ * Update theme with name or theme option and repaint the chart.
+ * @param theme Theme name or theme option.
*/
- private setTheme(): void {
- deprecateLog('ECharts#setTheme() is DEPRECATED in ECharts 3.0');
+ setTheme(theme: string | ThemeOption): void {
+ this._updateTheme(theme);
+ if (this._model) {
+ this._model.setTheme(this._theme);
+ // Force refresh to apply theme changes
+ updateMethods.prepareAndUpdate.call(this, {
+ type: 'themeChanged'
+ });
+ }
Review Comment:
If we provide an API `setTheme`, I think the behavior should better be
similar to `setOption`. that is, it modifies the model, and causes re-render
either immediately or in a lazy way.
But in the current implementation, re-render will not be triggered.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
