Skip to content

fix bug #11049 #11212

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.

Sign up for GitHub

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

Closed
yufeng04 wants to merge 1 commit into from
Closed

fix bug #11049 #11212

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/component/toolbox/feature/DataZoom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ echarts.registerPreprocessor(function (option) {
}

if (toolboxOpt && toolboxOpt.feature) {
var dataZoomOpt = toolboxOpt.feature.dataZoom;
var dataZoomOpt = toolboxOpt.feature.dataZoom || {};
Copy link
Copy Markdown
Member

@100pah 100pah Sep 19, 2019

Choose a reason for hiding this comment

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

  1. Add a default {} will make the condition at L281 no longer make sense. So only add a {} is not enough.

  2. Add a defualt {} will cause that dataZoom models are always added whatever toolbox.featrue.dataZoom declared. I am worried that might bring some other bug or bad cases.
    And event we fix this error throw in this way, there are other issue: if setOption changes the axes, the toolbox dataZoom will not follow and will also be buggy.

I think this issue is a little complicated:
Currently, making a hidden dataZoom model is performed in the preprocess stage,
which can not take the existing options into account. But they need that, because
they need the axes info.
If we want to resolve this kind of issue thoroughly, we might need another stage,
perhaps in setOption(the second round of setOption, only triggered inside),
to insert/update this kind of hidden model, where the existing model (like axes)
can be taken into account.

Anyway, before we resolve it totally, we can probably consider the feature toolbox dataZoom
only support being used in the first setOption.

// FIXME: If add dataZoom when setOption in merge mode,
// no axis info to be added. See `test/dataZoom-extreme.html`
addForAxis('xAxis', dataZoomOpt);
Expand Down