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

Use entityNearest() rather than entityNearestByXThenY() for LinePlot.entitiesAt() #3558

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mcintyret
Copy link

@mcintyret mcintyret commented Oct 16, 2020

Usinig entityNearestByXThenY causes some weird behaviour - if the cursor is marginally closer in the x domain to another point than the one the user intended to click on, that point will be the one that gets selected - even if it is hundreds of pixels away in the y domain.

Even worse, the tooltip-on-hover codepath just uses entityNearest(), so the intuitive nearest point has a tooltip, but then clicking selects a different point.

I went through the git history and couldn't see any reason why the "by x then y" approach was taken, but may well be missing something.

Hovering showing the nearest point:
image

After clicking at that exact same cursor location:
image

After this PR, the selected point is the expected one.

@blueprint-bot
Copy link

Use entityNearest() rather than entityNearestByXThenWhy() for LinePlot.entitiesAt()

Demo: quicktests | fiddle

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

Let's leave the "unused" method since it's public

if (entity != null) {
return [entity];
} else {
return [];
}
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we shouldn't delete this because technically this would be a public API break and require a major version bump

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.

None yet

3 participants