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 not closing #34001

Merged
merged 6 commits into from
May 12, 2021
Merged

Fix tooltip not closing #34001

merged 6 commits into from
May 12, 2021

Conversation

oscarkilhed
Copy link
Contributor

What this PR does / why we need it:
Fixes a bug where the tooltip on the bar graph stays open when the mouse leaves the plot.

@oscarkilhed oscarkilhed requested review from dprokop and a team and removed request for a team May 12, 2021 13:20
@oscarkilhed oscarkilhed marked this pull request as draft May 12, 2021 13:25
@oscarkilhed oscarkilhed removed the request for review from peterholmberg May 12, 2021 13:26
@@ -49,6 +49,11 @@ export const TooltipPlugin: React.FC<TooltipPluginProps> = ({

// Add uPlot hooks to the config, or re-add when the config changed
useLayoutEffect(() => {
if (plotCtx && plotCtx.plot) {
plotCtx.plot.root.onmouseleave = (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you may instead consider binding to .u-over, so it disappears when leaving the grid area instead of the entire chart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, where can I find .u-over? I'm a bit lost here and this solution was quite hacky, any guidance appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g.

const init = (u: uPlot) => {
let over = u.root.querySelector('.u-over')! as HTMLElement;
over.style.overflow = 'hidden';
over.appendChild(barMark);
};

(just make sure the querySelector only runs during init and not on every redraw)

Copy link
Contributor

Choose a reason for hiding this comment

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

i really should just expose this as u.over in future uPlot versions, even in my own demos i have to do this querySelector dance pretty often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I think I've cleaned this up a bit. Thanks for the help.

Comment on lines +55 to +63
if (plotCtx && plotCtx.plot) {
plotCtx.plot.root.querySelector('.u-over')!.addEventListener('mouseleave', plotMouseLeave);
}

return () => {
if (plotCtx && plotCtx.plot) {
plotCtx.plot.root.querySelector('.u-over')!.removeEventListener('mouseleave', plotMouseLeave);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Feel more comfortable now with the event listeners than with onmouseleave, good stuff!

@dprokop
Copy link
Member

dprokop commented May 12, 2021

Ah, and it works as expected!

@oscarkilhed oscarkilhed marked this pull request as ready for review May 12, 2021 17:52
@oscarkilhed oscarkilhed merged commit 1b5f1bf into main May 12, 2021
@oscarkilhed oscarkilhed deleted the oscark/fix-tooltip-not-closing branch May 12, 2021 17:52
ryantxu pushed a commit that referenced this pull request May 13, 2021
* Fix tooltip not closing

* make this a bit less hacky

* use a more specific element for the mouseleave event

* Make sure we limit running of effect

* Remove console log

* Probably don't need useCallback
mortenaa pushed a commit to mortenaa/grafana that referenced this pull request May 25, 2021
* Fix tooltip not closing

* make this a bit less hacky

* use a more specific element for the mouseleave event

* Make sure we limit running of effect

* Remove console log

* Probably don't need useCallback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants