Refactor scattergl selection #2311
Conversation
src/plots/cartesian/select.js
Outdated
| @@ -278,7 +278,7 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) { | |||
| else { | |||
| // TODO: remove in v2 - this was probably never intended to work as it does, | |||
| // but in case anyone depends on it we don't want to break it now. | |||
| gd.emit('plotly_selected', undefined); | |||
| gd.emit('plotly_selected', {points: [], range: null}); | |||
There was a problem hiding this comment.
That gets triggered whenever we click on a point in select/lasso mode. The event receives undefined data object, which makes user check it manually. Simple compatibility is better.
There was a problem hiding this comment.
Sure, simple compatibility is better. That's why we'll do this in v2. Until then, we must preserve backward compatibility. Please undo this change.
src/traces/scattergl/index.js
Outdated
| if(dragmode === 'lasso' || dragmode === 'select') { | ||
| if(scene.select2d && scene.selectBatch) { | ||
| scene.scatter2d.update(scene.unselectedOptions); | ||
| // update selection |
There was a problem hiding this comment.
Separated code chunks are in single location now
There was a problem hiding this comment.
On the gl2d_12 mock, calling Plotly.restyle(gd, 'selected.marker.color', 'blue') leads to:
There was a problem hiding this comment.
Should be fixed with 98cddf3
| @@ -82,6 +83,10 @@ var attrs = module.exports = overrideAll({ | |||
| marker: scatterAttrs.unselected.marker | |||
| }, | |||
|
|
|||
| opacity: extendFlat({}, plotAttrs.opacity, { | |||
| editType: 'calc' | |||
| }), | |||
There was a problem hiding this comment.
Looks good. Thanks!
I think the best way to test this would be to spy on ScatterGl.calc checking that Plotly.relayout(gd, 'opacity', /**/) does invoke it - similar to this suite.
| return Plotly.restyle(gd, {'opacity': 0.1}); | ||
| }) | ||
| .then(function() { | ||
| expect(ScatterGl.calc).toHaveBeenCalledTimes(2); |
|
Adding a test for #2298 will be tricky. @dfcreative is their a way to assert that some internal field (I guessing in |
|
@etpinard it is possible to test |
|
Wow. Nice job @dfcreative this is looking very solid. 💃 (cc @alexcjohnson ) |
|
... @dfcreative make sure to close all the related issues after you merge this. |
This enhances scattergl selection handling, fixing #2298.