You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2021/06/19 13:53:50 UTC
[GitHub] [echarts] ssthouse opened a new pull request #15194: fix: hovered tooltip not update when options' series data changed
ssthouse opened a new pull request #15194:
URL: https://github.com/apache/echarts/pull/15194
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
### Fixed issues
https://github.com/apache/echarts/issues/15188
## Details
### Before: What was the problem?
Originally, when we update the series' data, the shown tooltip will not update until we move the mouse.
After fixing, the tooltip will update, like below:
(the series data changed in the background 2 seconds after tooltip shows)
### After: How is it fixed in this PR?
Originally the `TooltipView` only checks if the series data's size changed, if the user didn't change size but the value in-place, the tooltip will not update.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] ssthouse commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
ssthouse commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r671064991
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -951,10 +955,20 @@ class TooltipView extends ComponentView {
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const series = self._ecModel.getSeriesByIndex(idxItem.seriesIndex);
+ const cbParams = series.getDataParams(idxItem.dataIndexInside) as TooltipCallbackDataParams;
Review comment:
fixed
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r671160313
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -945,16 +948,28 @@ class TooltipView extends ComponentView {
&& lastItem.axisId === thisItem.axisId
&& lastIndices.length === newIndices.length;
- contentNotChanged && each(lastIndices, function (lastIdxItem, j) {
+ contentNotChanged && each(lastIndices, (lastIdxItem, j) => {
const newIdxItem = newIndices[j];
contentNotChanged = contentNotChanged
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const cbParams = cbParamsList[idxItem.seriesIndex];
+ if (!cbParams) {
+ return;
+ }
+ if (lastCbParamsList[idxItem.seriesIndex].data !== cbParams.data) {
Review comment:
The code can be as simple as
```ts
if (cbParams && lastCbParamsList[idxItem.seriesIndex].data !== cbParams.data)
```
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#issuecomment-881420796
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r670988297
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -951,10 +955,20 @@ class TooltipView extends ComponentView {
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const series = self._ecModel.getSeriesByIndex(idxItem.seriesIndex);
+ const cbParams = series.getDataParams(idxItem.dataIndexInside) as TooltipCallbackDataParams;
Review comment:
I think it's better to use the `cbParamsList` in parameter instead of get a new one.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -923,11 +924,14 @@ class TooltipView extends ComponentView {
// FIXME
// Should we remove this but leave this to user?
- private _updateContentNotChangedOnAxis(dataByCoordSys: DataByCoordSys[]) {
+ private _updateContentNotChangedOnAxis(dataByCoordSys: DataByCoordSys[],
+ cbParamsList: TooltipCallbackDataParams[]) {
const lastCoordSys = this._lastDataByCoordSys;
+ const lastCbParamsList = this._cbParamsList;
let contentNotChanged = !!lastCoordSys
&& lastCoordSys.length === dataByCoordSys.length;
+ const self = this;
Review comment:
Can use arrow functions here.
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang merged pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #15194:
URL: https://github.com/apache/echarts/pull/15194
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#issuecomment-864409508
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] ssthouse commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
ssthouse commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r671212142
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -945,16 +948,28 @@ class TooltipView extends ComponentView {
&& lastItem.axisId === thisItem.axisId
&& lastIndices.length === newIndices.length;
- contentNotChanged && each(lastIndices, function (lastIdxItem, j) {
+ contentNotChanged && each(lastIndices, (lastIdxItem, j) => {
const newIdxItem = newIndices[j];
contentNotChanged = contentNotChanged
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const cbParams = cbParamsList[idxItem.seriesIndex];
+ if (!cbParams) {
+ return;
+ }
+ if (lastCbParamsList[idxItem.seriesIndex].data !== cbParams.data) {
Review comment:
got it, fixed~
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] ssthouse commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
ssthouse commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r671112188
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -945,16 +948,25 @@ class TooltipView extends ComponentView {
&& lastItem.axisId === thisItem.axisId
&& lastIndices.length === newIndices.length;
- contentNotChanged && each(lastIndices, function (lastIdxItem, j) {
+ contentNotChanged && each(lastIndices, (lastIdxItem, j) => {
const newIdxItem = newIndices[j];
contentNotChanged = contentNotChanged
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const cbParams = cbParamsList[idxItem.dataIndexInside];
Review comment:
fixed
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r671589994
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -945,16 +948,25 @@ class TooltipView extends ComponentView {
&& lastItem.axisId === thisItem.axisId
&& lastIndices.length === newIndices.length;
- contentNotChanged && each(lastIndices, function (lastIdxItem, j) {
+ contentNotChanged && each(lastIndices, (lastIdxItem, j) => {
const newIdxItem = newIndices[j];
contentNotChanged = contentNotChanged
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const cbParams = cbParamsList[idxItem.seriesIndex];
+ if (cbParams && lastCbParamsList[idxItem.seriesIndex].data !== cbParams.data) {
+ contentNotChanged = false;
+ }
+ });
});
});
this._lastDataByCoordSys = dataByCoordSys;
+ this._cbParamsList = cbParamsList;
return !!contentNotChanged;
Review comment:
`!!` is not necessary. It must be boolean here.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -945,16 +948,25 @@ class TooltipView extends ComponentView {
&& lastItem.axisId === thisItem.axisId
&& lastIndices.length === newIndices.length;
- contentNotChanged && each(lastIndices, function (lastIdxItem, j) {
+ contentNotChanged && each(lastIndices, (lastIdxItem, j) => {
const newIdxItem = newIndices[j];
contentNotChanged = contentNotChanged
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const cbParams = cbParamsList[idxItem.seriesIndex];
+ if (cbParams && lastCbParamsList[idxItem.seriesIndex].data !== cbParams.data) {
+ contentNotChanged = false;
+ }
+ });
Review comment:
The late review is just FYI.
1. This traversal `each` looks repeated with the above `each` ([L951-L956](https://github.com/ssthouse/echarts/blob/6321470854143c2ae82f574869a68f5651106072/src/component/tooltip/TooltipView.ts#L951-L956)). Maybe we can move this logic there as one of the conditions of `contentNotChanged`.
2. Assuming **1** is not required, it's better to avoid invoking `each` if `contentNotChanged` has been already `false` and should use `for` loop rather than `each` so that we can return once `contentNotChanged` becomes `false`.
3. We should consider the case that `seriesData` isn't changed but `seriesName` is changed, in which the tooltip should be also updated.
<img src="https://user-images.githubusercontent.com/26999792/126021868-a3548ec1-b3d5-4947-8bf2-b7a3479f29a1.gif" width="400" alt="seriesName change" title="`seriesName` change">
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #15194:
URL: https://github.com/apache/echarts/pull/15194#discussion_r671073329
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -945,16 +948,25 @@ class TooltipView extends ComponentView {
&& lastItem.axisId === thisItem.axisId
&& lastIndices.length === newIndices.length;
- contentNotChanged && each(lastIndices, function (lastIdxItem, j) {
+ contentNotChanged && each(lastIndices, (lastIdxItem, j) => {
const newIdxItem = newIndices[j];
contentNotChanged = contentNotChanged
&& lastIdxItem.seriesIndex === newIdxItem.seriesIndex
&& lastIdxItem.dataIndex === newIdxItem.dataIndex;
});
+
+ // check is cbParams data value changed
+ lastCbParamsList && zrUtil.each(lastItem.seriesDataIndices, (idxItem) => {
+ const cbParams = cbParamsList[idxItem.dataIndexInside];
Review comment:
It should be `idxItem.seriesIndex` here. It includes data of all series on one `dataIndex`. Also, we need to check if cbParams is null because the length of the last indices may not match the current.
--
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: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org