Skip to content

Sankey hoverinfo options not honored #3097

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

Closed
antoinerg opened this issue Oct 11, 2018 · 9 comments
Closed

Sankey hoverinfo options not honored #3097

antoinerg opened this issue Oct 11, 2018 · 9 comments
Labels
feature something new

Comments

@antoinerg
Copy link
Contributor

As of right now, Sankey traces ignore all hoverinfo options except for 'none' and 'skip'.

https://codepen.io/anon/pen/xyrXXm

@alexcjohnson
Copy link
Collaborator

In fact it looks like the fact that sankey even has a hoverinfo attribute is accidental, probably the whole attributes file was copied over from pie (judging from the flags) and that one never got deleted or addressed.

It's a bit of a tricky question how to handle hoverinfo here, as there are two independent classes of object you can hover on, links and nodes (speaking of which, we should probably have an attribute hoveron: 'links+nodes' here, and this comment applies to any other trace types with a hoveron flaglist). Links and nodes have some matching fields (flow value) and some different. What if you want links to show flow values but nodes not to.

What if hoverinfo in this case was split into link.hoverinfo and node.hoverinfo? Then we wouldn't need hoveron here, you could just specify the two hoverinfo values differently.

@antoinerg
Copy link
Contributor Author

antoinerg commented Oct 11, 2018

That makes sense. I guess we could also have the upcoming templates on hover be split into node.hovertemplate and link.hovertemplate 🤔

@etpinard What do you think? Should I go ahead and implement link and node separately?

@alexcjohnson
Copy link
Collaborator

That makes sense. I guess we could also have the upcoming templates on hover be split into node.hovertemplate and link.hovertemplate 🤔

Yes, I think those had better be separate too.

@antoinerg and I had a discussion about how to handle this for other types that support hoveron or otherwise have multiple classes of hoverable items - like scatter, how would we specify separate point and fill (and eventually line) hover templates (or separate hoverinfo and hoverlabel style)? Tentative conclusion was that trace.hover(info|template|label) should apply only to points, we should make a new container trace.fills.hover(info|template|label) for fill hover (and perhaps at least hoverlabel would be inherited from trace.hoverlabel) and when we get around to line hover we can add those to the existing line container, ie trace.line.hover(info|template|label). Any other ideas?

@antoinerg
Copy link
Contributor Author

Because all the flags for hoveron are plural (ex.: lines, points, fills), I think it should be nodes.(info|template|label) and links.(info|template|label). As @alexcjohnson pointed out to me, we already have an attribute named fill so making it plural for hover would prevent a collision.

@antoinerg
Copy link
Contributor Author

I wrote my last comment a bit too fast. We already have node and link attributes so I should just use them 🤔

@antoinerg antoinerg added feature something new and removed bug something broken labels Oct 16, 2018
@kristianjf
Copy link

One thing I observed, seemingly related to this issue, is node.hoverinfo only seems to include the sum of values for incoming or outgoing flow. Would it be possible to provide the option to manipulate this (incoming, outgoing, or both: incoming and outgoing)?

@antoinerg
Copy link
Contributor Author

Thank you @kristianjf for the suggestion! I agree that it should be included when Sankey's hoverinfo options are implemented.

@antoinerg
Copy link
Contributor Author

The hoverinfo feature discussed in this issue might become useless with the upcoming potential inclusion of hovertemplate from PR #3284.

What do you think @kristianjf @plotly/plotly_js ?

@gvwilson
Copy link
Contributor

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson

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

No branches or pull requests

4 participants