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 namelength=0 case + implement namelength to loneHover traces #3734

Merged
merged 8 commits into from Apr 9, 2019

Conversation

Projects
None yet
5 participants
@etpinard
Copy link
Member

commented Apr 5, 2019

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

Looks good me.
@antoinerg @nicolaskruchten any opinion on @etpinard question:

Do you think namelength should (essentially) override < extra > ?
@nicolaskruchten

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I think namelength should do nothing if extra is set in hovertemplate

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Hmm, I was going to argue the opposite - if you template some field into <extra>, it could still be useful to limit it with namelength. What if we just default to namelength: -1 when hovertemplate is used, but still honor it if provided?

@antoinerg

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

Hmm, I was going to argue the opposite - if you template some field into <extra>, it could still be useful to limit it with namelength. What if we just default to namelength: -1 when hovertemplate is used, but still honor it if provided?

I think I prefer @alexcjohnson suggestion above. I think it's both useful and more consistent with our current behavior. All hover labels should be stylable in the same way whether they use hovertemplate or not.

@etpinard I just realized that the two functions Fx.(lone|multi)Hover do not call cleanPoint which is where your fix takes place. Therefore, the current PR will not work with sunburst, sankey, parcats, webGL and possibly others. This begs the question: is there any reason we're not calling cleanPoint in functions Fx.(lone|multi)Hover?

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

This begs the question: is there any reason we're not calling cleanPoint in functions Fx.(lone|multi)Hover?

Ha, good eye namelength never got implemented for those traces, it looks like.

@nicolaskruchten

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Well, the namelength vs extra nomenclature is odd. Also labelling that widget in the RCE will be interesting if they interact.

I don't have a strong preference.

@antoinerg

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

Well, the namelength vs extra nomenclature is odd.

I regret using <extra> as a tag. <name> would have been more appropriate since it literally sets what by default is the trace name in hoverlabels. Should we start supporting both tag names?

As for namelength, it should do what the doc says:

Sets the default length (in number of characters) of the trace name in the hover labels for all traces.

etpinard added some commits Apr 8, 2019

add noHover trace module category,
use it to determine if we coerce `hoverinfo` and `hoverlabel`
in Plots.supplyTraceDefaults
honour hoverlabel.namelength when hovertemplate is set
... while making hoverlabel.namelength dflt to -1
    when hovertemplate is set
implement hoverlabel.namelength for traces using Fx.loneHover
... that is pie, sankey, sunburst and all gl3d traces
@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

This PR is ready again for review.


What if we just default to namelength: -1 when hovertemplate is used, but still honor it if provided?

Done in e9f8403

Therefore, the current PR will not work with sunburst, sankey, parcats, webGL and possibly others.

namelength should now work for all our trace type that support hoverlabel.* thanks to 918d22e and 7f1645d

@etpinard etpinard added this to the v1.47.0 milestone Apr 8, 2019

@@ -70,6 +71,7 @@ function render(scene) {
if(lastPicked !== null) {
var pdata = project(scene.glplot.cameraParams, selection.dataCoordinate);
trace = lastPicked.data;
var traceNow = gd._fullData[trace.index];

This comment has been minimized.

Copy link
@archmoj

archmoj Apr 8, 2019

Collaborator

@etpinard Could you please describe why traceNow is needed here?
Thanks in advance.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 8, 2019

Author Member

Because hoverlabel.* edits don't (and shouldn't have to) replot the scene. As Plots.supplyDefaults creates new gd._fullData and gd._fullLayout refs on every call, we need to use the right "now" trace object when hovering.

@antoinerg

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

Thanks for all the tests! It looks good to me and I think it is ready to go. I will let @archmoj give you a dancer if he is satisfied with your answer.

Absolutely non-blocking but could you set hoverlabel.namelength to -1 in the mock sunburst_values to make sure the hover labels stay the same? Right now the trace names are clipped.

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

@etpinard thanks for the PR.
💃
Also - To help Q&A you may modify the PR description & briefly describe what it does.

@etpinard etpinard changed the title Fix namelength=0 case Fix namelength=0 case + implement namelength to loneHover traces Apr 9, 2019

@etpinard etpinard merged commit adf7ca2 into master Apr 9, 2019

10 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-bundle Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine3 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@etpinard etpinard deleted the namelength-zero-fix branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.