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

bugfix: issue 11801 #11849

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sunjh
Copy link

@sunjh sunjh commented Aug 2, 2024

Add some extra check to make sure the tooltip be hidden while cursor moving outside of the chart. This is to fix the issue #11801

const options = this.options.setContext(this.getContext());
const active = this._active;
let properties;
let tooltipItems = [];

if (!active.length) {
if (!active.length || !inChartArea) {
Copy link
Collaborator

@LeeLenaleee LeeLenaleee Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this behavior was added intentionally and is not a bug when I look at the code and the pr where this got introduced.

I will look at this later to see if this might break any implementations that rely on that behavior

image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback.
Correct me if I'm wrong, from my understanding, the !active.length is added to make sure the chart can be interactive even when the cursor is out of it (hovering over the legend and high light the corresponding data in the chart).
Adding the extra check will only effect the visibility of the tooltip in this scenario.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LeeLenaleee, do you think this is a proper change or do you have any further comments on this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I tested the changes, what you said is correct, this does still work with externally calling code to setActiveElements. Although the name is kind of misleading in those cases. Since the bool is called inChartArea, but when you use setActiveElements you are not in the chart area.

Also this change seems to introduce a new bug. When you move your mouse to the legend quickly and return to the same slice the hover does not trigger anymore

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LeeLenaleee , I tested it and tried to fix that, but looks like it's not a simple change. I will change this PR to draft and work on this continuously.

@sunjh sunjh marked this pull request as draft August 27, 2024 07:07
Signed-off-by: Saber Sun <saber.sun@fmr.com>
@kayhern kayhern force-pushed the bugfix/issue-11801-tooltip-visible-on-legend-hover branch from 5f1d470 to 66a5204 Compare August 30, 2024 13:25
@sunjh sunjh marked this pull request as ready for review August 30, 2024 15:47
@sunjh sunjh marked this pull request as draft September 27, 2024 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants