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 for #1124 - fixed donut innerradius hovering trigger #1125

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

Conversation

hmaidasani
Copy link

Submitting a fix for issue #1124.

The original flot.pie.js renamed to jquery.flot.pie.orig.js would fire a pie hover event on a nearby slice when hovering inside the innerradius or whitespace of a donut pie chart. When hovering inside the donut, a hover or highlighting should not occur. Added the fix to the file jquery.flot.pie.js. The bug and fix are shown in a working demo in file bugTest.html. The demo shows the bug in an iframe of file bug.html and the fix is shown in an iframe of file bugfix.html.

@dnschnur
Copy link
Member

Thanks for your contribution, but there are several problems:

  1. Supporting files, like bug.html, bugTest.html, etc. should be linked in the comments, not included in the pull request. They don't belong in the codebase. There's also no need to include a .orig version; we can see the diff.
  2. Your second commit adds a whole new gradient feature that is unrelated to the original fix, and isn't mentioned in the description. It belongs in a separate pull request, though there are already some pull requests for this feature.
  3. The fix itself is very fragile; you're doing DOM operations that won't always return what you expect, and are inefficient. I think the proper solution would be to use isPointInPath, as the plugin already does for the outer circle, except with the inner circle instead.

If you fix these and rebase, I'd be happy to merge; I agree that this is a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants