fix(tooltip): fix tooltip content does not refresh when changing tooltip trigger from 'axis' to 'item'#20710
Conversation
|
Thanks for your contribution! |
|
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20710@0698205 |
src/component/tooltip/TooltipView.ts
Outdated
| x: payload.x, | ||
| y: payload.y | ||
| }); | ||
| if (tooltipModel.option.trigger === 'axis') { |
There was a problem hiding this comment.
Can you explain why this action should only be dispatched for axis triggers?
There was a problem hiding this comment.
I found this action causes the emphasis state lost.
After dispatch:
Expected:
Maybe dispatch this action only when axisPointer is enabled would be better.
Is there any way to check if axisPointer is enabled or not?
There was a problem hiding this comment.
Logically speaking, I don't think whether the axisPointer is enabled makes any difference in this regard. If we consider that setOption should lead to an update of the tooltip, then it is also logical that setOption causes the emphasis state to become updated.
This is also one of the reasons why I'm more inclined to think that the issue in this PR isn't a bug. Perhaps keeping the setting of "changing the 'trigger' via setOption won't trigger the redrawing of the tooltip" would simplify matters. May I ask what initially led you to need to fix the problem in this PR?
There was a problem hiding this comment.
I will revert this change because you think losing the "emphasis" is not a bug.
But changing the tooltip "trigger" without updating the tooltip is certainly a bug.
|
@Ovilia I've updated the code accordingly. Could you help to review again? Thanks! |
|
@Ovilia @100pah @plainheart Could someone help to review? Thanks! |
Ovilia
left a comment
Ovilia
left a comment
There was a problem hiding this comment.
Sorry for the late reply! We've been quite busy working towards 6.0.
src/component/tooltip/TooltipView.ts
Outdated
| const api = this._api; | ||
| const triggerOn = tooltipModel.get('triggerOn'); | ||
|
|
||
| if (tooltipModel.option.trigger !== 'axis') { |
There was a problem hiding this comment.
May I ask why we should check if it's axis here? What happens if we don't use this if? Because I don't see other places checking this before setting _lastDataByCoordSys to be null.
There was a problem hiding this comment.
_lastDataByCoordSys is used for axis tooltip only
If current trigger is still axis, it should not be cleared.
Other cases to clear the _lastDataByCoordSys
- When the tooltip is hidden (regardless of trigger mode)
- In
TooltipView. _tryShow
- Showing axis tooltip via
_showAxisTooltip() - Or other tooltip (set to
null)
|
@Ovilia @100pah @plainheart Could someone help to review? Thanks! |
1 similar comment
|
@Ovilia @100pah @plainheart Could someone help to review? Thanks! |
|
@plainheart I've updated the PR accordingly. Could you help to review again? Thanks! |
|
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
plainheart
changed the title
trigger from 'axis' to 'item'
3 participants
Brief Information
This pull request is in the type of:
What does this PR do?
fix: Changing tooltip trigger from axis to item doesn't refresh the displayed tooltip
Fixed issues
#20706
Details
Before: What was the problem?
After: How does it behave after the fixing?
Document Info
One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information