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/07/17 01:58:22 UTC
[GitHub] [echarts] plainheart commented on a change in pull request #15194: fix: hovered tooltip not update when options' series data changed
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