-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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.
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
fix: hightlight multiple series not work as expected #15207
Conversation
Thanks for your contribution! |
The fix looks neat! One suggestion: this test case can be put in |
fixed~ |
src/core/echarts.ts
Outdated
if (!excludeSeriesIdMap || excludeSeriesIdMap.get(model.id) == null) { | ||
if (isHighDownPayload(payload)) { | ||
if (model instanceof SeriesModel) { | ||
if (payload.type === HIGHLIGHT_ACTION_TYPE && !payload.notBlur) { | ||
// only blur once | ||
const isFirstTimeBlur = modelIndex === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some cases modelndex
may not be 0
. It’s the index in the option. For example: if we change the test code to:
chart.dispatchAction({
type: 'highlight',
seriesIndex: [1],
});
The modelIndex
of the first iteration will be 1
. And no series will be blurred, which is not expected.
I think a better solution is using a flag to check if any series triggers blur.
There is another situation I think we need to consider:
If we set blurScope
to series
. Only other data in the same series will be blurred. Should we still use the "only one series will trigger blur" policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another unrelated suggestion:
I think the old case you are using can be added to hoverFocus.html
. It's more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pissang Hi~
In some cases modelndex may not be 0. It’s the index in the option.
I have changed to flag implementation now~
(I was thinking that the second params index
always starts with zero like Array.forEach
. )
There is another situation I think we need to consider:
If we set blurScope to series. Only other data in the same series will be blurred. Should we still use the "only one series will trigger blur" policy?
I think when blurScope === 'series'
, we can stick to the original blur all logic.
I think the old case you are using can be added to hoverFocus.html. It's more clear.
👌 I have moved the old testcase into the hoverFocus.html
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I was thinking that the second params index always starts with zero like Array.forEach. )
Yeah, this API design may be misleading.
I think when blurScope === 'series', we can stick to the original blur all logic.
There is another option blurScope: 'coordinateSystem'
may have a similar issue. I'm thinking if we can support multiple series in
Line 513 in 4e671b6
export function blurSeriesFromHighlightPayload( |
Line 408 in 4e671b6
export function blurSeries( |
Perhaps it's a better place to do this complex logic.
BTW: this comment can be removed to avoid confusion.
Line 457 in 4e671b6
// TODO blurScope: coordinate system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pissang I moved the logic into echarts/src/util/states.ts
;
(ps: the code modification in src/core/echarts.ts
now is to improve code readability)
And for blurScope: 'coordinateSystem'
, a new testcase is added in hoverFocus.html
only blur once when blurScope!="series"
bc3399e
to
e193f6a
Compare
src/util/states.ts
Outdated
@@ -437,7 +438,10 @@ export function blurSeries( | |||
|
|||
const blurredSeries: SeriesModel[] = []; | |||
|
|||
ecModel.eachSeries(function (seriesModel) { | |||
ecModel.eachSeries(function (seriesModel, seriesIndex) { | |||
if (excludedSeriesIndexes && excludedSeriesIndexes.includes(seriesIndex)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
includes
is not supported in IE 9+, which we still supported currently. We can use zrUtil.indexOf(excludedSeriesIndexes, seriesIndex) >= 0
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/util/states.ts
Outdated
@@ -529,11 +532,12 @@ export function blurSeriesFromHighlightPayload( | |||
el = data.getItemGraphicEl(current++); | |||
} | |||
} | |||
const excludedSeriesIndexes = payload.seriesIndex || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seriesIndex
is one of the possible queries. Developers can also use payload.seriesId
to query target series. Perhaps we need to query the highlight seriesModel
list in echarts.ts
and pass it to this function.
Also, I'm not sure if there is a logic issue:
If we have focus: 'self'
in one series and dispatch a highlight
action on this series. This series will also be in this excludedSeriesIndexes
and not perform the focus: 'self'
strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm not sure if there is a logic issue:
If we have focus: 'self' in one series and dispatch a highlight action on this series. This series will also be in this excludedSeriesIndexes and not perform the focus: 'self' strategy.
Yes, There is a logic issue. I add another testcase for this.
Please take a look if this is the expected result~
src/util/states.ts
Outdated
@@ -409,7 +409,8 @@ export function blurSeries( | |||
targetSeriesIndex: number, | |||
focus: InnerFocus, | |||
blurScope: BlurScope, | |||
api: ExtensionAPI | |||
api: ExtensionAPI, | |||
excludedSeriesIndexes?: number[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be indices instead of indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
test/data/basicChartsOptions.js
Outdated
emphasis: coordinateSystemBlurEmphasis, | ||
}, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems these new cases are mainly target for the hover focus feature. We can append them in hoverFocus.html
instead of adding them in the basicChartOptions
. Options in basicChartOptions
are usually very basic and will be used in tests of many features.
Also, if we want to add a new test case, always adding it at the tail instead of the head. Or it will affect the replay of user interactions in the automatic visual regression testing tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, fixed~
- fix indexed to indices - move testcase from basicChartsOptions -> hoverFocus.html
3f222af
to
9c3a81c
Compare
src/util/states.ts
Outdated
@@ -454,7 +457,8 @@ export function blurSeries( | |||
|| blurScope === 'coordinateSystem' && !sameCoordSys | |||
// Not blur self series if focus is series. | |||
|| focus === 'series' && sameSeries | |||
// TODO blurScope: coordinate system | |||
// Not blur other series if focus is self | |||
|| focus === 'self' && !sameSeries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to blur other series if blurScope
is 'global' and focus
is 'self'. You can run visual regression test(VRT in short) on hoverFocus.html
, hoverFocus2.html
to see if anythings breaks. The command of starting VRT is:
npm run test:visual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pissang Hi,I found it's complicated to first blur then highlight for each series, like this;
for(const series of seriesList){
blurNecessaryPart(series);
highlightCurrent(series);
}
Since the previous highlighted series can be easily blurred by the latter series;
It may be more clear if we first blur all the necessary parts then hight all the necessary parts, like this;
for(const series of seriesList){
blurNecessaryPart(series);
}
for(const series of seriesList){
highlightCurrent(series);
}
I modified the current code to this scenario and tested with npm run test:visual
, please take a look~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssthouse That's brilliant! It should work well. I can't find other issues. The only thing that needs to be done is recording the user interactions on the new add test case so we can test it automatically.
You can click the Record Interaction
on the VRT page button to open the recording page. Then follow the hint on the page and use the shortcut key to start recording. During the recording, you can click buttons, hover elements. Each click will trigger a screenshot to compare, but hover won't. So if you want to compare the current picture, you should at least click or use the shortcut key to trigger the screenshot.
Usually, each action needs to focus on one test case and the interaction count in each action should be as less as possible. Feel free to ask me if you have any problem with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pissang I have recorded the testcases and tried to run them.
Currently, it failed(as expected), is there anything I should do to make it pass?
267f22f
to
bd77c90
Compare
bd77c90
to
e8a9a0f
Compare
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?
Fixed issues
Close #15204
Details
Before: What was the problem?
Dispatch
hightlight
action with the following code only highlight the lastest series; The expected result is all three series are highlighted;After: How is it fixed in this PR?
Fix the logic of running blur unHighlighted series multiple times;
Misc
Related test cases or examples to use the new APIs
NA.
Others
Merging options
Other information