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

hovertemplate support for sankey #3284

Merged
merged 3 commits into from Dec 7, 2018
Merged

Conversation

antoinerg
Copy link
Contributor

Closes #3269

@antoinerg
Copy link
Contributor Author

antoinerg commented Nov 23, 2018

I am not sure the description of hovertemplate attributes is satisfactory. I'd like to discuss the changes made in commit d952713.

Finally, because the inclusion hovertemplate in Fx.loneHover calls have been tested in other traces, is it necessary to test it again in sankey_test.js? Is it OK to only check for the existence of the expected variables in eventData? 🤔

@@ -146,6 +147,10 @@ var attrs = module.exports = overrideAll({
].join(' ')
},
hoverlabel: fxAttrs.hoverlabel, // needs editType override,
hovertemplate: hovertemplateAttrs({}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Sankey event data is a bit of mess:

image

image

There's probably way more stuff it in than we need. Moreover, some of it isn't consistent with event data for other traces e.g. the trace key should be fullData. I'm not how much we can change it w/o making breaking changes.

Oh well, these descriptions look ok to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added fullData to the eventData in commit 3204274

Copy link
Contributor

Choose a reason for hiding this comment

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

v2 changes tracked in #168 (comment)

@etpinard
Copy link
Contributor

Finally, because the inclusion hovertemplate in Fx.loneHover calls have been tested in other traces, is it necessary to test it again in sankey_test.js?

I would add one "true" hovertemplate test case where we check that a user-defined hovertemplate gives the correct hover label content. You're right though, no need to re-test all the loneHover logic again 👌

@antoinerg
Copy link
Contributor Author

I would add one "true" hovertemplate test case where we check that a user-defined hovertemplate gives the correct hover label content.

Done in commit 3204274!

@etpinard etpinard added this to the v1.43.0 milestone Dec 7, 2018
@etpinard
Copy link
Contributor

etpinard commented Dec 7, 2018

@antoinerg oops I forgot about this PR.

Looks good 💃

@antoinerg antoinerg merged commit 8ce56d7 into master Dec 7, 2018
@antoinerg antoinerg deleted the 3269-sankey-hovertemplate branch December 7, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants