Skip to content
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(tooltip): keep axis tooltip open on refresh #13100

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

dr-itz
Copy link
Contributor

@dr-itz dr-itz commented Aug 7, 2020

Brief Information

This pull request is in the type of:

  • bug fixing

What does this PR do?

Fix a tooltip disappearing when tooltip trigger is "axis" and data is refreshed.

Fixed issues

Details

Before: What was the problem?

Chart with datazoom slider and live update every few seconds. The slider is somewhere in the middle with a fixed value range. When the user hovers over the data, the tooltip with trigger "axis" shows correctly. But as soon as new data is pushed to the chart the tooltip disappears. This PR fixes that.

After: How is it fixed in this PR?

Tooltip with trigger axis stays when data is refresehd.

Usage

Are there any API changes?

  • The API has been changed.

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

@susiwen8
Copy link
Contributor

@dr-itz Thanks, but could you make a PR to next branch?

@dr-itz
Copy link
Contributor Author

dr-itz commented Aug 17, 2020

@dr-itz Thanks, but could you make a PR to next branch?

sure, will do. Out of curiosity, will there be another 4.x.y release?

@susiwen8
Copy link
Contributor

@dr-itz 4.9 is the last one which will be released in few days.

@dr-itz
Copy link
Contributor Author

dr-itz commented Aug 17, 2020

@dr-itz 4.9 is the last one which will be released in few days.

thanks for letting me know. I'd really appreciate this PR here to be part of 4.9 since it's something we no proper workaround for.

Of course I'll do the fix for next as well

@susiwen8
Copy link
Contributor

I'm afraid that's too late, 4.9 has been voted through, so we cannot add any feature or fix in this release @dr-itz

@susiwen8
Copy link
Contributor

If this feature were really urgent to you, I suggest to build ECharts by yourself. @dr-itz
@Ovilia @100pah @pissang Should we add this PR in 5.0?

@dr-itz
Copy link
Contributor Author

dr-itz commented Aug 18, 2020

I only just realized that there is a _lastDataByCoordSys already, so this might be simpler than this.

Btw 😍 the move to TypeScript in next

@dr-itz dr-itz force-pushed the fix/keep-axis-tooltip branch from d1ab266 to 3319561 Compare August 18, 2020 13:05
@dr-itz
Copy link
Contributor Author

dr-itz commented Aug 18, 2020

I only just realized that there is a _lastDataByCoordSys already, so this might be simpler than this.

changed it to use that. It's simpler and works as expected. I also tested the non-axis trigger and couldn't find any problem. I'm not familiar enough with this to see why _lastDataByCoordSys was cleared in render(), so I'm not sure about that one. I think _tryShow() and manuallyHideTip() should clear it when necessary.

I'll rebase for next later 😉

@dr-itz dr-itz force-pushed the fix/keep-axis-tooltip branch from 3319561 to b367718 Compare August 25, 2020 14:07
@dr-itz dr-itz changed the base branch from master to next August 25, 2020 14:07
@dr-itz
Copy link
Contributor Author

dr-itz commented Aug 25, 2020

sorry for the delay..I finally found some time and rebased this to next

@pissang
Copy link
Contributor

pissang commented Aug 26, 2020

LGTM. Clearing _lastDataByCoordSys seems to be a mistake. Thanks for the work! @dr-itz

@pissang pissang merged commit ed19857 into apache:next Aug 26, 2020
@echarts-bot
Copy link

echarts-bot bot commented Aug 26, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants