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

adding hovertemplates to polar bar and scatter and ternary scatter #3398

Merged
merged 4 commits into from
Jan 14, 2019

Conversation

nicolaskruchten
Copy link
Contributor

I mostly just followed the pattern in #3126 :)

@etpinard etpinard added this to the 1.44.0 milestone Jan 4, 2019
@etpinard
Copy link
Contributor

etpinard commented Jan 4, 2019

Nice. Would you be interested in adding tests to help us 🔒 down this feature?

@nicolaskruchten
Copy link
Contributor Author

Any recommendation about where to add these tests? Is there a pattern somewhere for testing hovers?

@nicolaskruchten
Copy link
Contributor Author

OK, I added tests for scatterpolar and scatterpolargl because the test harness was set up for it. I tried to replicate the test harness for barpolar and scatterternary but I wasn't able to trick assertHoverLabelContent into finding anything :(

@etpinard
Copy link
Contributor

etpinard commented Jan 7, 2019

I tried to replicate the test harness for barpolar and scatterternary but I wasn't able to trick

Right, because barpolar and scatterternary hover test don't actually test Fx.hover (where the template substitutions happen). They instead test their trace module's .hoverPoints method - which only feeds in pointData.hovertemplate to Fx.hover.

So, an ok test for both barpolar and scatterternary would be to assert that pointData.hovertemplate is defined after hoverPoints().

@nicolaskruchten
Copy link
Contributor Author

OK, done :)

@etpinard
Copy link
Contributor

Nice PR! 💃 - thanks for your help @nicolaskruchten !

@nicolaskruchten nicolaskruchten merged commit ebeddb5 into master Jan 14, 2019
@nicolaskruchten
Copy link
Contributor Author

Happy to help! I might submit a followup for scattergeo and scattermapbox if they're also using the easy case.

@nicolaskruchten nicolaskruchten deleted the more_hovertemplates branch January 14, 2019 14:47
@etpinard
Copy link
Contributor

etpinard commented Jan 14, 2019

I might submit a followup for scattergeo and scattermapbox

Yeah, those should be straight-forward and similar to this PR.

Trace types scatter3d, surface, mesh3d and parcats might be more of a pain (but doable), their implementation would be similar to pie in #3126

@VeraZab
Copy link

VeraZab commented Jan 22, 2019

so nice to see it automagically work in RCE :)

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

Successfully merging this pull request may close these issues.

3 participants