-
Notifications
You must be signed in to change notification settings - Fork 329
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
Add hoverColor option #141
Conversation
Thanks, fixed now |
Is there a reason we want events to fire still? That seems non-intuitive to me and I'm wondering if that could be disabled |
@Techn1x this PR will need to be rebased |
Yes, in my case, I hide/show annotations when they are hovered on and off, so still need events to fire. Perhaps the option could be renamed to be more clear, and/or have a second option named something else that removes the whole annotation (but that would probably be a different PR) |
Hi, I'm about to add chartjs-plugin-annotation to a project and I would like this functionality (I need to conditionally add a line depending on where the chart is rendered from), is this PR likely to be merged soon? |
It seems unintuitive to me that you can hover over a hidden object to make it appear. I think it'd be confusing if things just started magically appearing. How would you know where you're supposed to hover? |
@amiedavis Conditionally adding and removing annotations can currently be done by adding/removing them from the annotations array, it doesn't necessarily need this PR |
@Techn1x thanks, that's how I'm doing it at the moment I just thought it might be neater if this was available. I will continue on without it :) |
That feature is pretty cool, but I think it's probably a bit outside the scope of the plugin, so I'm going to close this PR for now |
To clarify, I'm fine with the display option, but I think if an annotation is hidden it should not fire events. We can reopen if you're interested in pursuing that |
Fair enough. So we're agreed that at least a With regards to hidden annotations, would you support either of these approaches?
Setting an annotation to transparent could be done externally by setting each color, but is tedious if managing several annotations which could have different colors to remember & reapply each time you want to change the transparency I think approach 2 is interesting because it could allow implementations other than my own, and is somewhat consistent with chart.js |
I'll reopen this PR and you can decide whether to update this one or start a new one. Feel free to close it again in the latter case |
@Techn1x will you have a chance to update this PR? |
@benmccann I'm very sorry, shortly after january I switched employment and no longer use chartjs, and that knowledge has mostly evacuated my brain :) |
Discussed here: #95
display
is set to true by default. Setting to false will hide either all annotations, or the individual annotation. Hiding the annotation essentially just makes it invisible, so that events will still trigger (useful for hover events, for example)