Skip to content

Range slider fixes (user set y axis types + trace clearance) #1472

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

Merged
merged 7 commits into from
Mar 14, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 10 additions & 2 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,19 @@ Plotly.plot = function(gd, data, layout, config) {
uid = trace.uid;

if(!isVisible || !Registry.traceIs(trace, '2dMap')) {
fullLayout._paper.selectAll(
var query = (
'.hm' + uid +
',.contour' + uid +
',#clip' + uid
).remove();
);

fullLayout._paper
.selectAll(query)
.remove();

fullLayout._infolayer.selectAll('g.rangeslider-container')
.selectAll(query)
.remove();
}

if(!isVisible || !trace._module.colorbar) {
Expand Down
5 changes: 5 additions & 0 deletions src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout)
.remove();
}
}

oldFullLayout._infolayer.selectAll('g.rangeslider-container')
.select('g.scatterlayer')
.selectAll('g.trace')
.remove();
}

var hadCartesian = (oldFullLayout._has && oldFullLayout._has('cartesian'));
Expand Down
26 changes: 14 additions & 12 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -571,23 +571,25 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou
if(oldUid === newTrace.uid) continue oldLoop;
}

// clean old heatmap, contour, and scatter traces
//
// Note: This is also how scatter traces (cartesian and scatterternary) get
// removed since otherwise the scatter module is not called (and so the join
// doesn't register the removal) if scatter traces disappear entirely.
var query = (
'.hm' + oldUid +
',.contour' + oldUid +
',#clip' + oldUid +
',.trace' + oldUid
);

// clean old heatmap, contour traces and clip paths
// that rely on uid identifiers
if(hasPaper) {
oldFullLayout._paper.selectAll(
'.hm' + oldUid +
',.contour' + oldUid +
',#clip' + oldUid +
',.trace' + oldUid
).remove();
oldFullLayout._paper.selectAll(query).remove();
}

// clean old colorbars
// clean old colorbars and range slider plot
if(hasInfoLayer) {
oldFullLayout._infolayer.selectAll('.cb' + oldUid).remove();

oldFullLayout._infolayer.selectAll('g.rangeslider-container')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose this pattern

fullLayout._infolayer.selectAll('g.rangeslider-container')

over keeping a ref to some underscore key e.g. fullLayout._rangeSliders to make these additions work even when there are no range sliders on the graph without having to resort to Registry.

.select(query).remove();
}
}
};
Expand Down
6 changes: 5 additions & 1 deletion src/traces/contour/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ function plotOne(gd, plotinfo, cd) {
heatmapPlot(gd, plotinfo, [cd]);
}
// in case this used to be a heatmap (or have heatmap fill)
else fullLayout._paper.selectAll('.hm' + uid).remove();
else {
fullLayout._paper.selectAll('.hm' + uid).remove();
fullLayout._infolayer.selectAll('g.rangeslider-container')
.selectAll('.hm' + uid).remove();
}

makeCrossings(pathinfo);
findAllPaths(pathinfo);
Expand Down
2 changes: 2 additions & 0 deletions src/traces/heatmap/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ function plotOne(gd, plotinfo, cd) {

// in case this used to be a contour map
fullLayout._paper.selectAll('.contour' + uid).remove();
fullLayout._infolayer.selectAll('g.rangeslider-container')
.selectAll('.contour' + uid).remove();

if(trace.visible !== true) {
fullLayout._paper.selectAll('.' + id).remove();
Expand Down
84 changes: 84 additions & 0 deletions test/jasmine/tests/range_slider_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,90 @@ describe('the range slider', function() {
})
.then(done);
});

it('should clear traces in range plot when needed', function(done) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson This commit fixes part 1

the data is gone from the graph, it should disappear from the range slider too. If you delete any but the last trace, the range slider does update correctly, but not when you delete the last one.

of #1473 in a pretty ugly way, unfortunately.

Most trace clearance (i.e. deletion or visible: false or type-restyle) is handled via this block:

plotinfo.plot.selectAll('g:not(.scatterlayer)').selectAll('g.trace').remove();

except for scatter, heatmap and contour traces. That is, part 1 of #1473 actually only affected scatter, heatmap and contour traces.

Scatter traces are exempted from that selectAll('').remove() block since @rreusser's animation PR to smooth out their data updates. They are cleared here. Contour and heatmap groups don't always match one-to-one with trace items (e.g. a contour trace can have a heatmap fill group) so we use their uid to tell which trace to clear here, here here and here.

In brief, this commit ensure that every special .remove() call mentioned in the previous paragraph clear traces in the range slider containers as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ugly but... ya gotta do what ya gotta do. Nice tests. 👍


function count(query) {
return d3.select(getRangeSlider()).selectAll(query).size();
}

Plotly.plot(gd, [{
type: 'scatter',
x: [1, 2, 3],
y: [2, 1, 2]
}, {
type: 'bar',
x: [1, 2, 3],
y: [2, 5, 2]
}], {
xaxis: {
rangeslider: { visible: true }
}
})
.then(function() {
expect(count('g.scatterlayer > g.trace')).toEqual(1);
expect(count('g.barlayer > g.trace')).toEqual(1);

return Plotly.restyle(gd, 'visible', false);
})
.then(function() {
expect(count('g.scatterlayer > g.trace')).toEqual(0);
expect(count('g.barlayer > g.trace')).toEqual(0);

return Plotly.restyle(gd, 'visible', true);
})
.then(function() {
expect(count('g.scatterlayer > g.trace')).toEqual(1);
expect(count('g.barlayer > g.trace')).toEqual(1);

return Plotly.deleteTraces(gd, [0, 1]);
})
.then(function() {
expect(count('g.scatterlayer > g.trace')).toEqual(0);
expect(count('g.barlayer > g.trace')).toEqual(0);

return Plotly.addTraces(gd, [{
type: 'heatmap',
z: [[1, 2, 3], [2, 1, 3]]
}]);
})
.then(function() {
expect(count('g.imagelayer > g.hm')).toEqual(1);

return Plotly.restyle(gd, 'visible', false);
})
.then(function() {
expect(count('g.imagelayer > g.hm')).toEqual(0);

return Plotly.restyle(gd, {
visible: true,
type: 'contour'
});
})
.then(function() {
expect(count('g.maplayer > g.contour')).toEqual(1);

return Plotly.restyle(gd, 'type', 'heatmap');
})
.then(function() {
expect(count('g.imagelayer > g.hm')).toEqual(1);
expect(count('g.maplayer > g.contour')).toEqual(0);

return Plotly.restyle(gd, 'type', 'contour');
})
.then(function() {
expect(count('g.imagelayer > g.hm')).toEqual(0);
expect(count('g.maplayer > g.contour')).toEqual(1);

return Plotly.deleteTraces(gd, [0]);
})
.then(function() {
expect(count('g.imagelayer > g.hm')).toEqual(0);
expect(count('g.maplayer > g.contour')).toEqual(0);
})
.then(done);

});
});

describe('handleDefaults function', function() {
Expand Down