sankey: implement node grouping via mouse selection #3712
Conversation
| if(node.partOfGroup) continue; // Those are invisible | ||
|
|
||
| // TODO: decide on selection criteria, using centroid for now | ||
| var pos = [(node.x0 + node.x1) / 2, (node.y0 + node.y1) / 2]; |
There was a problem hiding this comment.
Ok 👍 I 🔪 the TODO in 20d8c92
Maybe we should add a visual cue to let the user know a given node is now part of a selection 🤔 This could also be added later on.
src/traces/sankey/base_plot.js
Outdated
| }; | ||
|
|
||
| dragOptions[i].prepFn = function(e, startX, startY) { | ||
| prepSelect(e, startX, startY, dragOptions[i], dragMode); |
There was a problem hiding this comment.
... and I guess prepSelect does the right thing when dragmode isn't select nor lasso?
There was a problem hiding this comment.
Ahh. This is tricky actually for sankey, dragmode values zoom and pan don't really make sense. I'm not sure if we can do anything about it in v1. Would it be ok to have zoom and pan correspond to the current "drag-on-nodes" behaviour?
There was a problem hiding this comment.
Would it be ok to have zoom and pan correspond to the current "drag-on-nodes" behavior?
Yes, I think it would be OK. I will return early in subplotUpdateFx when dragMode is either zoom or pan.
| @@ -284,6 +286,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) { | |||
| dragOptions.mergedPolygons.length = 0; | |||
| [].push.apply(dragOptions.mergedPolygons, mergedPolygons); | |||
| } | |||
|
|
|||
| doneFnCompleted(selection); | |||
There was a problem hiding this comment.
I like this solution a lot. Very clean 🥇
|
Looking good @antoinerg - some minor comments, but you're for sure on the right track 🏎️ |
src/components/modebar/buttons.js
Outdated
| icon: Icons.home, | ||
| click: function(gd) { | ||
| Registry.call('restyle', gd, { | ||
| 'node.groups': [[]], |
There was a problem hiding this comment.
As we discussed in person, it might be better to restyle back to the "initial" view rather than to just remove all the groups.
To do so:
There was a problem hiding this comment.
@antoinerg let me know if you need help implementing this 😄
There was a problem hiding this comment.
call restyle with the values in that stash each in the modebar button handler (see example)
I don't think I can reuse resetView because it does a relayout whereas we need a restyle.
There was a problem hiding this comment.
Right, no need to use resetView() found in modebar/buttons.js, but you should try to replicate the same pattern.
There was a problem hiding this comment.
Done in commit 506da4c! Let me know if it is satisfactory.
I will now write Jasmine tests for the reset button. Is there anything else you would like me to add?
There was a problem hiding this comment.
I will now write Jasmine tests for the reset button. Is there anything else you would like me to add?
Nop, once that jasmine test is in, this one will be good to go!
antoinerg
force-pushed
the
sankey-selection2
branch
from
1f0282b to
506da4c
Compare
|
Excellent! You might not feel that way @antoinerg as this PR didn't add that many lines to Well-earned 💃 |
Closes #3582