Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Range slider second iteration (part 2: on-par range plots !!!) #1017

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 4 commits into from
Oct 11, 2016
Merged

Range slider second iteration (part 2: on-par range plots !!!) #1017

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0de9760
cartesian: factor per-subplot plot command
etpinard Oct 6, 2016
e2db9ec
rangeslider: replace buggy & incomplete range slider plotting routine
etpinard Oct 6, 2016
1b627a5
test: update range slider mocks
etpinard Oct 6, 2016
c7de743
test: add two more range slider mocks
etpinard Oct 6, 2016
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
Prev Previous commit
Next Next commit
rangeslider: replace buggy & incomplete range slider plotting routine 
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
  • Loading branch information
@etpinard
etpinard committed Oct 6, 2016
commit e2db9ec79626371479c1be2f7c7444bbb69adbbe
119 changes: 109 additions & 10 deletions 119 src/components/rangeslider/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,18 @@ var d3 = require('d3');

var Plotly = require('../../plotly');
var Plots = require('../../plots/plots');
var Axes = require('../../plots/cartesian/axes');

var Lib = require('../../lib');
var Drawing = require('../drawing');
var Color = require('../color');

var Cartesian = require('../../plots/cartesian');
var Axes = require('../../plots/cartesian/axes');

var dragElement = require('../dragelement');
var setCursor = require('../../lib/setcursor');

var constants = require('./constants');
var rangePlot = require('./range_plot');


module.exports = function(gd) {
Expand Down Expand Up @@ -55,7 +59,14 @@ module.exports = function(gd) {
.classed(constants.containerClassName, true)
.attr('pointer-events', 'all');

rangeSliders.exit().remove();
// remove exiting sliders and their corresponding clip paths
rangeSliders.exit().each(function(axisOpts) {
var rangeSlider = d3.select(this),
opts = axisOpts[constants.name];

rangeSlider.remove();
fullLayout._topdefs.select('#' + opts._clipId).remove();
});

// remove push margin object(s)
if(rangeSliders.exit().size()) clearPushMargins(gd);
Expand All @@ -82,6 +93,8 @@ module.exports = function(gd) {
domain = axisOpts.domain;

opts._id = constants.name + axisOpts._id;
opts._clipId = opts._id + '-' + fullLayout._uid;

opts._width = graphSize.w * (domain[1] - domain[0]);
opts._height = (fullLayout.height - margin.b - margin.t) * opts.thickness;
opts._offsetShift = Math.floor(opts.borderwidth / 2);
Expand All @@ -95,6 +108,7 @@ module.exports = function(gd) {

rangeSlider
.call(drawBg, gd, axisOpts, opts)
.call(addClipPath, gd, axisOpts, opts)
.call(drawRangePlot, gd, axisOpts, opts)
.call(drawMasks, gd, axisOpts, opts)
.call(drawSlideBox, gd, axisOpts, opts)
Expand Down Expand Up @@ -279,17 +293,102 @@ function drawBg(rangeSlider, gd, axisOpts, opts) {
});
}

function drawRangePlot(rangeSlider, gd, axisOpts, opts) {
var rangePlots = rangePlot(gd, opts._width, opts._height);
function addClipPath(rangeSlider, gd, axisOpts, opts) {
var fullLayout = gd._fullLayout;

var gRangePlot = rangeSlider.selectAll('g.' + constants.rangePlotClassName)
var clipPath = fullLayout._topdefs.selectAll('#' + opts._clipId)
.data([0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet. I'll do that in a PR of its own. Hopefully this week.


gRangePlot.enter().append('g')
.classed(constants.rangePlotClassName, true);
clipPath.enter().append('clipPath')
.attr('id', opts._clipId)
.append('rect')
.attr({ x: 0, y: 0 });

clipPath.select('rect').attr({
width: opts._width,
height: opts._height
});
}

function drawRangePlot(rangeSlider, gd, axisOpts, opts) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the algorithm is as follows:

  1. find all the subplots spanned by the axis where the range slider reside
  2. for each subplot, mock a plotinfo object (which are essentially the cartesian subplot state objects) using range slider options to find the width and height
  3. find all traces in this subplot
  4. Call Cartesian.rangePlot which creates all the required layers and plots / styles the given plotinfo subplot
  5. clip the subplot

var subplotData = Axes.getSubplots(gd, axisOpts),
calcData = gd.calcdata;

var rangePlots = rangeSlider.selectAll('g.' + constants.rangePlotClassName)
.data(subplotData, Lib.identity);

rangePlots.enter().append('g')
.attr('class', function(id) { return constants.rangePlotClassName + ' ' + id; })
.call(Drawing.setClipUrl, opts._clipId);

rangePlots.order();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice. The order is important to manage (and easy to neglect) with all of these d3 joins.


rangePlots.exit().remove();

var mainplotinfo;

rangePlots.each(function(id, i) {
var plotgroup = d3.select(this),
isMainPlot = (i === 0);

var oppAxisOpts = Axes.getFromId(gd, id, 'y'),
oppAxisName = oppAxisOpts._name;

var mockFigure = {
Copy link
Contributor

@rreusser rreusser Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

data: [],
layout: {
xaxis: {
domain: [0, 1],
range: opts.range.slice()
},
width: opts._width,
height: opts._height,
margin: { t: 0, b: 0, l: 0, r: 0 }
}
};

mockFigure.layout[oppAxisName] = {
domain: [0, 1],
range: oppAxisOpts.range.slice()
};

Plots.supplyDefaults(mockFigure);

var xa = mockFigure._fullLayout.xaxis,
ya = mockFigure._fullLayout[oppAxisName];

var plotinfo = {
id: id,
plotgroup: plotgroup,
xaxis: xa,
yaxis: ya
};

if(isMainPlot) mainplotinfo = plotinfo;
else {
plotinfo.mainplot = 'xy';
plotinfo.mainplotinfo = mainplotinfo;
}

Cartesian.rangePlot(gd, plotinfo, filterRangePlotCalcData(calcData, id));

if(isMainPlot) plotinfo.bg.call(Color.fill, opts.bgcolor);
});
}

function filterRangePlotCalcData(calcData, subplotId) {
var out = [];

for(var i = 0; i < calcData.length; i++) {
var calcTrace = calcData[i],
trace = calcTrace[0].trace;

if(trace.xaxis + trace.yaxis === subplotId) {
out.push(calcTrace);
}
}

gRangePlot.html(null);
gRangePlot.node().appendChild(rangePlots);
return out;
}

function drawMasks(rangeSlider, gd, axisOpts, opts) {
Expand Down
24 changes: 0 additions & 24 deletions 24 src/components/rangeslider/helpers.js
View file
Open in desktop

This file was deleted.

179 changes: 0 additions & 179 deletions 179 src/components/rangeslider/range_plot.js
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions 6 src/plots/cartesian/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ exports.drawFramework = function(gd) {
});
};

exports.rangePlot = function(gd, plotinfo, cdSubplot) {
makeSubplotLayer(plotinfo);
plotOne(gd, plotinfo, cdSubplot);
Plots.style(gd);
Copy link
Contributor

@rreusser rreusser Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

function makeSubplotData(gd) {
var fullLayout = gd._fullLayout,
subplots = Axes.getSubplots(gd);
Expand Down
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.