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

Calculate legend positioning relative to closest data point #1013

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

acontreras89
Copy link
Contributor

This is done by checking the current highlighted series.

This is done by checking the current highlighted series
@mirabilos mirabilos merged commit d92cd5a into danvk:master Dec 9, 2022
@acontreras89
Copy link
Contributor Author

Hey @mirabilos, thanks for merging our PR! 🎉

After some time using our own version of the library (which included this change), we spotted a little bug 🐛 eventually point would be undefined, causing the following error:

plot-zoom-out-of-sync-error

Not sure why this was happening,* we fixed it by simply adding a guard in commit redradix@1a74f4d.

* It is possible that when the user is moving the mouse, we try to calculate the next legend position before the highlight series has been updated. In this case, a point for the highlighted series may not be available 🤷‍♂️

@mirabilos
Copy link
Collaborator

Hi @acontreras89, interesting. Why not simply default to points[0] then? Is there any negative effect to expect from returning early, or will the code just be called again once points are available again?

@acontreras89
Copy link
Contributor Author

@mirabilos falling back to points[0] would most likely be a better idea.

Our use case is very sensitive in terms of performance, so we opted for the guard assuming this was an unnecessary legend computation (following on my previous comment.)

But honestly, I think we simply did not put that much effort into figuring out what was really happening. I just wanted to let you know this can happen now that the PR (finally) got merged 🙃

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