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

Fixes issue #8 Move tooltip with mouse pointer #157

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rohchakr
Copy link

Added event mousemove to call the function tooltip.show(). Now within the frame, the tooltip will follow the mouse pointer.

@rohchakr rohchakr mentioned this pull request Apr 25, 2020
Copy link
Owner

@spiermar spiermar left a comment

Choose a reason for hiding this comment

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

Worried about the overhead of using mousemove. It's called very frequently. Not sure if a debounce would even help.
The issue was also created when d3-tip was the default library. Don't think this would help with d3-tip and not as big of an issue now with the new tooltip.

Now, the tooltip overflowing to outside the screen is a bigger issue.

src/flamegraph.js Show resolved Hide resolved
src/flamegraph.js Outdated Show resolved Hide resolved
if (tooltip) tooltip.hide()
detailsHandler(null)
}).on('mousemove', function (d) {
const chartBoundingClientRect = svg._groups[0][0].parentNode.getBoundingClientRect()
const chartRightBoundary = chartBoundingClientRect.x + chartBoundingClientRect.width
Copy link
Author

Choose a reason for hiding this comment

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

This might be moved up as a separate function to fetch chart boundary

Copy link
Owner

Choose a reason for hiding this comment

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

Ok

@rohchakr
Copy link
Author

@spiermar
If mousemove seems to be risky and not of high priority, then we can again revisit it sometime later on.
For now, if the approach provided in latest commit to resolve tooltip overflowing the chart right boundary is acceptable, let's make it clean and let me know if I need to revert the changes for mousemove

@rohchakr
Copy link
Author

tooltip1tooltip2

@rohchakr rohchakr requested a review from spiermar April 29, 2020 13:19
@spiermar spiermar self-assigned this May 11, 2020
@spiermar
Copy link
Owner

Need to move chartRightBoundary to tooltip to maintain .show compatibility with d3-tip.

Copy link
Owner

@spiermar spiermar left a comment

Choose a reason for hiding this comment

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

Need to move chartRightBoundary to tooltip to maintain .show compatibility with d3-tip.

src/flamegraph.js Outdated Show resolved Hide resolved
if (tooltip) tooltip.hide()
detailsHandler(null)
}).on('mousemove', function (d) {
const chartBoundingClientRect = svg._groups[0][0].parentNode.getBoundingClientRect()
Copy link
Owner

Choose a reason for hiding this comment

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

What rect is this exactly?

Copy link
Author

Choose a reason for hiding this comment

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

Using the DOM API getBoundingClientRect() to get the size and position of the chart area rendered in the web page.

if (tooltip) tooltip.hide()
detailsHandler(null)
}).on('mousemove', function (d) {
const chartBoundingClientRect = svg._groups[0][0].parentNode.getBoundingClientRect()
const chartRightBoundary = chartBoundingClientRect.x + chartBoundingClientRect.width
Copy link
Owner

Choose a reason for hiding this comment

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

Ok


let tooltipLeft = event.pageX + 10

if (tooltipRightBoundary > chartRightBoundary) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we completely flip the tooltip to the right if boundary is exceeded? Not just shift so it fits?

Copy link
Author

Choose a reason for hiding this comment

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

This is a good idea. Let me try...

@rohchakr
Copy link
Author

Working to make it compatible with d3-tip as per https://github.com/caged/d3-tip/blob/master/docs/showing-and-hiding-tooltips.md#tipshow

so that the function is called like tip.show(d, this) like before.

@rohchakr
Copy link
Author

rohchakr commented May 16, 2020

@spiermar I was wondering about the reason why we want our .show to be compatible with d3-tip. d3-tip.js seems to be a standard and we would want users to be comfortable using flamegraph.tooltip in the same way as they would use d3-tip. It might be painful to work with libraries which don't have api's consistent with standards.

Is there any other reason?

Calculating chart boundary inside tooltip.js and keeping the method .show compatible with d3-tip
@rohchakr
Copy link
Author

Need to move chartRightBoundary to tooltip to maintain .show compatibility with d3-tip.

This is done.

@rohchakr rohchakr requested a review from spiermar May 16, 2020 11:48
@spiermar
Copy link
Owner

@spiermar I was wondering about the reason why we want our .show to be compatible with d3-tip. d3-tip.js seems to be a standard and we would want users to be comfortable using flamegraph.tooltip in the same way as they would use d3-tip. It might be painful to work with libraries which don't have api's consistent with standards.

Is there any other reason?

The change that dropped d3-tip from the dependencies in favor of the simple tooltip script happened recently and most users still use de-tip and will likely continue for a while, even if it was abandoned. Looking to not introduce breaking changes.

var tooltip = null
var html = defaultLabel
const rootElement = select('body')
const chartElement = select('#chart')
Copy link
Owner

Choose a reason for hiding this comment

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

This should be passed to the tooltip init, since it can be something else.

@spiermar spiermar removed their assignment Sep 22, 2021
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