fix: hightlight multiple series not work as expected by ssthouse · Pull Request #15207 · apache/echarts

ssthouse · 2021-06-22T05:14:37Z

Brief Information

This pull request is in the type of:

bug fixing
new feature
others

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;

dispatchAction({
    type: 'highlight',
    seriesIndex: [0, 1, 2],
});

After: How is it fixed in this PR?

Fix the logic of running blur unHighlighted series multiple times;

Misc

The API has been changed (apache/echarts-doc#xxx).
This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

NA.

Others

Merging options

Please squash the commits into a single one when merge.

Other information

echarts-bot · 2021-06-22T05:14:40Z

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

pissang · 2021-06-22T09:29:56Z

The fix looks neat! One suggestion: this test case can be put in hoverFocus2.html. It has similar cases

ssthouse · 2021-06-22T09:48:05Z

The fix looks neat! One suggestion: this test case can be put in hoverFocus2.html. It has similar cases

fixed~

pissang · 2021-06-22T10:43:29Z

                        if (model instanceof SeriesModel) {
-                            if (payload.type === HIGHLIGHT_ACTION_TYPE && !payload.notBlur) {
+                            // only blur once
+                            const isFirstTimeBlur = modelIndex === 0;


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?

Another unrelated suggestion:

I think the old case you are using can be added to hoverFocus.html. It's more clear.

@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.

When blurScope="series"

When blurScope!="series"

(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

echarts/src/util/states.ts

Line 513 in 4e671b6

export function blurSeriesFromHighlightPayload(

and

echarts/src/util/states.ts

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.

echarts/src/util/states.ts

Line 457 in 4e671b6

// TODO blurScope: coordinate system

@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"

pissang · 2021-06-24T06:45:44Z


-    ecModel.eachSeries(function (seriesModel) {
+    ecModel.eachSeries(function (seriesModel, seriesIndex) {
+        if (excludedSeriesIndexes && excludedSeriesIndexes.includes(seriesIndex)) {


includes is not supported in IE 9+, which we still supported currently. We can use zrUtil.indexOf(excludedSeriesIndexes, seriesIndex) >= 0 instead.

pissang · 2021-06-24T06:51:53Z

            el = data.getItemGraphicEl(current++);
        }
    }
+    const excludedSeriesIndexes = payload.seriesIndex || [];


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.

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~

pissang · 2021-06-24T06:52:40Z

    blurScope: BlurScope,
-    api: ExtensionAPI
+    api: ExtensionAPI,
+    excludedSeriesIndexes?: number[]


Should be indices instead of indexes.

pissang · 2021-06-24T06:55:06Z

+                emphasis: coordinateSystemBlurEmphasis,
+            },
+        ],
+    },


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.

got it, fixed~

- fix indexed to indices - move testcase from basicChartsOptions -> hoverFocus.html

pissang · 2021-06-29T02:50:37Z

            || focus === 'series' && sameSeries
-            // TODO blurScope: coordinate system
+            // Not blur other series if focus is self
+            || focus === 'self' && !sameSeries


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

@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~

@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.

@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?

echarts-bot · 2021-07-02T13:11:50Z

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

fix: hightlight multiple series not work as expected

af34cf8

echarts-bot Bot added the PR: awaiting review label Jun 22, 2021

pull-request-size Bot added the size/XS label Jun 22, 2021

pissang added this to the 5.2 milestone Jun 22, 2021

pissang requested review from 100pah and pissang June 22, 2021 07:51

refactor: move hightlight multiple series testcase to hoverFocus2.html

52adb46

pissang requested changes Jun 22, 2021

View reviewed changes

echarts-bot Bot added PR: revision needed PR: awaiting review and removed PR: awaiting review labels Jun 22, 2021

fix: wrong blur status when dispatch hightlight multiple series action

4e671b6

only blur once when blurScope!="series"

echarts-bot Bot removed the PR: revision needed label Jun 23, 2021

pull-request-size Bot added size/M and removed size/XS labels Jun 23, 2021

feat: move blur multipe series logic into util/state.ts

e193f6a

ssthouse force-pushed the fix/multiple-series-highlight branch from bc3399e to e193f6a Compare June 24, 2021 05:27

pissang requested changes Jun 24, 2021

View reviewed changes

echarts-bot Bot added PR: revision needed PR: awaiting review and removed PR: awaiting review PR: revision needed labels Jun 24, 2021

pull-request-size Bot added size/L and removed size/M labels Jun 25, 2021

fix: focus=self hightlight issue

9c3a81c

- fix indexed to indices - move testcase from basicChartsOptions -> hoverFocus.html

ssthouse force-pushed the fix/multiple-series-highlight branch from 3f222af to 9c3a81c Compare June 26, 2021 11:46

pissang requested changes Jun 29, 2021

View reviewed changes

echarts-bot Bot added PR: revision needed PR: awaiting review and removed PR: awaiting review PR: revision needed labels Jun 29, 2021

pull-request-size Bot added size/M and removed size/L labels Jul 2, 2021

ssthouse force-pushed the fix/multiple-series-highlight branch from 267f22f to bd77c90 Compare July 2, 2021 02:30

feat: split highlight and blur step when handle hightlight action

e8a9a0f

ssthouse force-pushed the fix/multiple-series-highlight branch from bd77c90 to e8a9a0f Compare July 2, 2021 02:42

feat: add new visual testcases for hoverFocus.html

a232adc

pissang approved these changes Jul 2, 2021

View reviewed changes

pissang merged commit d14ccfc into apache:master Jul 2, 2021

echarts-bot Bot removed the PR: awaiting review label Jul 2, 2021

plainheart mentioned this pull request Aug 23, 2021

ECharts 中支持的图表行为 highlight 不能高亮多个系列吗 #15577

Closed

Conversation

ssthouse commented Jun 22, 2021 • edited by susiwen8 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief Information

What does this PR do?

Fixed issues

Details

Before: What was the problem?

After: How is it fixed in this PR?

Misc

Related test cases or examples to use the new APIs

Others

Merging options

Other information

Uh oh!

echarts-bot Bot commented Jun 22, 2021

Uh oh!

pissang commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ssthouse commented Jun 22, 2021

Uh oh!

pissang Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pissang Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pissang Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echarts-bot Bot commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ssthouse commented Jun 22, 2021 •

edited by susiwen8

Loading

pissang commented Jun 22, 2021 •

edited

Loading

pissang Jun 22, 2021 •

edited

Loading

pissang Jun 23, 2021 •

edited

Loading

pissang Jul 2, 2021 •

edited

Loading