Range slider second iteration (part 2: on-par range plots !!!) #1017
Conversation
- .. into a plotOne function that works with any sufficiently mocked plotinfo objects and calcdata array
by Cartesian.rangePlot which uses the same subplot layer creation and plot command as 'regular' cartesian subplots. - rm obsolete rangeslider/range_plot.js and rangeslider/helpers.js - add test for clipPath deletion
- the new rangePlot routine generates much sharper lines
etpinard
changed the title
| }); | ||
| } | ||
|
|
||
| function drawRangePlot(rangeSlider, gd, axisOpts, opts) { |
There was a problem hiding this comment.
So, the algorithm is as follows:
- find all the subplots spanned by the axis where the range slider reside
- for each subplot, mock a
plotinfoobject (which are essentially the cartesian subplot state objects) using range slider options to find the width and height - find all traces in this subplot
- Call
Cartesian.rangePlotwhich creates all the required layers and plots / styles the givenplotinfosubplot - clip the subplot
|
|
||
| var gRangePlot = rangeSlider.selectAll('g.' + constants.rangePlotClassName) | ||
| var clipPath = fullLayout._topdefs.selectAll('#' + opts._clipId) | ||
| .data([0]); |
There was a problem hiding this comment.
Did you manage to factor out the .data([0]) pattern? I suppose it would be a function with a name like var clipPath = createOrAppend(fullLayout._topdefs, '#' + opts._clipId);?
There was a problem hiding this comment.
Not yet. I'll do that in a PR of its own. Hopefully this week.
| .attr('class', function(id) { return constants.rangePlotClassName + ' ' + id; }) | ||
| .call(Drawing.setClipUrl, opts._clipId); | ||
|
|
||
| rangePlots.order(); |
There was a problem hiding this comment.
👍 Nice. The order is important to manage (and easy to neglect) with all of these d3 joins.
| var oppAxisOpts = Axes.getFromId(gd, id, 'y'), | ||
| oppAxisName = oppAxisOpts._name; | ||
|
|
||
| var mockFigure = { |
There was a problem hiding this comment.
Interesting. There's something I always like the usage of supplyDefaults for mocking instead of the real thing. Feels robust and lightweight. I liked using it with animations since many things just worked.
| exports.rangePlot = function(gd, plotinfo, cdSubplot) { | ||
| makeSubplotLayer(plotinfo); | ||
| plotOne(gd, plotinfo, cdSubplot); | ||
| Plots.style(gd); |
There was a problem hiding this comment.
Only tangentially relevant, but I'm not a huge fan of Plots.style. Long story short, when you transition the position and styles of an element at the same time, you must do them in a single transition operation, otherwise one discards the other.
In other words, for most d3 things that animate, I had to copy the styling into the inner join loop along with positioning instead of just applying styles on a completely separate pass (but to make sure it's robust and since the cost is probably small, I didn't remove styling from module.style).
It's fine as-is, just something to think about since the separate-pass styling approach wasn't fully compatible with what I needed.
There was a problem hiding this comment.
Yep, I'm not a fan of Plots.style too.
As soon as we'll find a solution for removing this ugly block
if(plotinfo.plot) {
plotinfo.plot.selectAll('g:not(.scatterlayer)').selectAll('g.trace').remove();
}in the main cartesian plot routine, we'll get rid of Plots.style
Yep exactly. This PR makes use of #946 to reuse the cartesian plot step for the range slider range plots. |
fixes #472
This PR builds on top of #946 and #962 to bring range plots (the little little subplot within the range slider) on-par with good old cartesian subplots.
That is, ALL cartesian trace types are now supported in the range slider - they now appear as they would in a miniature subplot.