Skip to content

scattergl visible restyle fix #2442

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 6, 2018
Merged

scattergl visible restyle fix #2442

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5c6576a
fix #2441 - don't attempt to draw empty option items
etpinard Mar 5, 2018
187ac23
fixup - no need to override opacity editType twice
etpinard Mar 5, 2018
91cac2e
DRY up scattergl
etpinard Mar 5, 2018
687a1f5
Merge branch 'master' into scattergl-visible-fix
etpinard Mar 6, 2018
a98d1d5
bring back scene.update + :lock: down its used during drag
etpinard Mar 6, 2018
4776832
no need to extendFlat attrs that 'only' get overrode by overrideAll
etpinard Mar 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/traces/scattergl/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,7 @@ var attrs = module.exports = overrideAll({
marker: scatterAttrs.unselected.marker
},

opacity: extendFlat({}, plotAttrs.opacity, {
editType: 'calc'
}),
opacity: extendFlat({}, plotAttrs.opacity)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't even need the extendFlat, right? overrideAll will take care of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 4776832


}, 'calc', 'nested');

Expand Down
17 changes: 10 additions & 7 deletions src/traces/scattergl/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,21 +556,24 @@ function sceneUpdate(gd, subplot) {
scene.draw = function draw() {
var i;
for(i = 0; i < scene.count; i++) {
if(scene.fill2d) scene.fill2d.draw(i);
if(scene.fill2d && scene.fillOptions[i]) {
// must do all fills first
scene.fill2d.draw(i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clear improvement over master - but this does raise the question of whether this (not your change, but the structure surrounding it) is really the right behavior, particularly with fill: 'tonext' but also with multiple fills to zero. I can try this out tomorrow and see if my interpretation of what this is doing is accurate, but In svg scatter, each fill gets drawn below any trace that it's attached to (even the trace before it if it's a fill: 'tonext') but above any other trace that comes before it. If all the lines and points are above all the fills, you can get points and lines that look disconnected from their own fills.

}
}
for(i = 0; i < scene.count; i++) {
if(scene.line2d) {
if(scene.line2d && scene.lineOptions[i]) {
scene.line2d.draw(i);
}
if(scene.error2d) {
if(scene.error2d && scene.errorXOptions[i]) {
scene.error2d.draw(i);
}
if(scene.error2d && scene.errorYOptions[i]) {
scene.error2d.draw(i + scene.count);
}
if(scene.scatter2d) {
if(scene.scatter2d && scene.markerOptions[i] && (!scene.selectBatch || !scene.selectBatch[i])) {
// traces in no-selection mode
if(!scene.selectBatch || !scene.selectBatch[i]) {
scene.scatter2d.draw(i);
}
scene.scatter2d.draw(i);
}
}

Expand Down
42 changes: 42 additions & 0 deletions test/jasmine/tests/gl2d_plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,48 @@ describe('@gl Test gl2d plots', function() {
.then(done);
});

it('should be able to toggle trace with different modes', function(done) {
Plotly.newPlot(gd, [{
// a trace with all regl2d objects
type: 'scattergl',
y: [1, 2, 1],
error_x: {value: 10},
error_y: {value: 10},
fill: 'tozeroy'
}, {
type: 'scattergl',
mode: 'markers',
y: [0, 1, -1]
}])
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
spyOn(scene.fill2d, 'draw');
spyOn(scene.line2d, 'draw');
spyOn(scene.error2d, 'draw');
spyOn(scene.scatter2d, 'draw');

return Plotly.restyle(gd, 'visible', 'legendonly', [0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On master, this gives:

peek 2018-03-05 17-29

})
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
expect(scene.fill2d.draw).toHaveBeenCalledTimes(0);
expect(scene.line2d.draw).toHaveBeenCalledTimes(0);
expect(scene.error2d.draw).toHaveBeenCalledTimes(0);
expect(scene.scatter2d.draw).toHaveBeenCalledTimes(1);

return Plotly.restyle(gd, 'visible', true, [0]);
})
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
expect(scene.fill2d.draw).toHaveBeenCalledTimes(1);
expect(scene.line2d.draw).toHaveBeenCalledTimes(1);
expect(scene.error2d.draw).toHaveBeenCalledTimes(2, 'twice for x AND y');
expect(scene.scatter2d.draw).toHaveBeenCalledTimes(3, 'both traces have markers');
})
.catch(fail)
.then(done);
});

it('@noCI should display selection of big number of regular points', function(done) {
// generate large number of points
var x = [], y = [], n = 2e2, N = n * n;
Expand Down