Skip to content

Shape clean up #378

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 5 commits into from
Apr 7, 2016
Merged

Shape clean up #378

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c53303b
Split method shapes.draw into smaller functions
n-riesco Apr 1, 2016
f67ff4d
Remove unnecessary step from shapes.drawAll()
n-riesco Apr 4, 2016
22d7248
Add more shapes tests
n-riesco Apr 6, 2016
f8dbdd1
Update shapes test
n-riesco Apr 7, 2016
92e46e3
Update shapes test
n-riesco Apr 7, 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
136 changes: 79 additions & 57 deletions src/components/shapes/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ function linearToData(ax) { return ax.type === 'category' ? ax.l2c : ax.l2d; }

shapes.drawAll = function(gd) {
var fullLayout = gd._fullLayout;
fullLayout._shapelayer.selectAll('path').remove();
for(var i = 0; i < fullLayout.shapes.length; i++) {
shapes.draw(gd, i);
}
Expand All @@ -115,86 +114,106 @@ shapes.add = function(gd) {
// if opt is blank, val can be 'add' or a full options object to add a new
// annotation at that point in the array, or 'remove' to delete this one
shapes.draw = function(gd, index, opt, value) {
var layout = gd.layout,
fullLayout = gd._fullLayout,
i;

// TODO: abstract out these drawAll, add, and remove blocks for shapes and annotations
if(!isNumeric(index) || index===-1) {
// no index provided - we're operating on ALL shapes
if(!index && Array.isArray(value)) {
// a whole annotation array is passed in
// (as in, redo of delete all)
layout.shapes = value;
shapes.supplyLayoutDefaults(layout, fullLayout);
shapes.drawAll(gd);
replaceAllShapes(gd, value);
return;
}
else if(value==='remove') {
// delete all
delete layout.shapes;
fullLayout.shapes = [];
shapes.drawAll(gd);
deleteAllShapes(gd);
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice add a few test cases for adding and removing shapes.

Here's the official way to add/remove shapes (and annotations for that matter): http://codepen.io/etpinard/pen/xVPoYG

return;
}
else if(opt && value!=='add') {
// make the same change to all shapes
for(i = 0; i < fullLayout.shapes.length; i++) {
shapes.draw(gd, i, opt, value);
}
updateAllShapes(gd, opt, value);
return;
}
else {
// add a new empty annotation
index = fullLayout.shapes.length;
fullLayout.shapes.push({});
index = gd._fullLayout.shapes.length;
gd._fullLayout.shapes.push({});
}
}

if(!opt && value) {
if(value==='remove') {
fullLayout._shapelayer.selectAll('[data-index="'+index+'"]')
.remove();
fullLayout.shapes.splice(index,1);
layout.shapes.splice(index,1);
for(i=index; i<fullLayout.shapes.length; i++) {
fullLayout._shapelayer
.selectAll('[data-index="'+(i+1)+'"]')
.attr('data-index',String(i));

// redraw all shapes past the removed one,
// so they bind to the right events
shapes.draw(gd,i);
}
deleteShape(gd, index);
return;
}
else if(value==='add' || Plotly.Lib.isPlainObject(value)) {
fullLayout.shapes.splice(index,0,{});
insertShape(gd, index, value);
}
}

var rule = Plotly.Lib.isPlainObject(value) ?
Plotly.Lib.extendFlat({}, value) :
{text: 'New text'};
updateShape(gd, index, opt, value);
};

if(layout.shapes) {
layout.shapes.splice(index, 0, rule);
} else {
layout.shapes = [rule];
}
function replaceAllShapes(gd, newShapes) {
gd.layout.shapes = newShapes;
shapes.supplyLayoutDefaults(gd.layout, gd._fullLayout);
shapes.drawAll(gd);
}

for(i=fullLayout.shapes.length-1; i>index; i--) {
fullLayout._shapelayer
.selectAll('[data-index="'+(i-1)+'"]')
.attr('data-index',String(i));
shapes.draw(gd,i);
}
}
function deleteAllShapes(gd) {
delete gd.layout.shapes;
gd._fullLayout.shapes = [];
shapes.drawAll(gd);
}

function updateAllShapes(gd, opt, value) {
for(var i = 0; i < gd._fullLayout.shapes.length; i++) {
shapes.draw(gd, i, opt, value);
}
}

function deleteShape(gd, index) {
gd._fullLayout._shapelayer.selectAll('[data-index="' + index + '"]')
.remove();

gd._fullLayout.shapes.splice(index, 1);

gd.layout.shapes.splice(index, 1);

for(var i = index; i < gd._fullLayout.shapes.length; i++) {
// redraw all shapes past the removed one,
// so they bind to the right events
gd._fullLayout._shapelayer
.selectAll('[data-index="' + (i+1) + '"]')
.attr('data-index', String(i));
shapes.draw(gd, i);
}
}

function insertShape(gd, index, newShape) {
gd._fullLayout.shapes.splice(index, 0, {});

var rule = Plotly.Lib.isPlainObject(newShape) ?
Plotly.Lib.extendFlat({}, newShape) :
{text: 'New text'};

if(gd.layout.shapes) {
gd.layout.shapes.splice(index, 0, rule);
} else {
gd.layout.shapes = [rule];
}

for(var i = gd._fullLayout.shapes.length - 1; i > index; i--) {
gd._fullLayout._shapelayer
.selectAll('[data-index="' + (i - 1) + '"]')
.attr('data-index', String(i));
shapes.draw(gd, i);
}
}

function updateShape(gd, index, opt, value) {
var i;

// remove the existing shape if there is one
fullLayout._shapelayer.selectAll('[data-index="'+index+'"]').remove();
gd._fullLayout._shapelayer.selectAll('[data-index="' + index + '"]')
.remove();

// remember a few things about what was already there,
var optionsIn = layout.shapes[index];
var optionsIn = gd.layout.shapes[index];

// (from annos...) not sure how we're getting here... but C12 is seeing a bug
// where we fail here when they add/remove annotations
Expand Down Expand Up @@ -261,8 +280,8 @@ shapes.draw = function(gd, index, opt, value) {
optionsIn[posAttr] = position;
}

var options = handleShapeDefaults(optionsIn, fullLayout);
fullLayout.shapes[index] = options;
var options = handleShapeDefaults(optionsIn, gd._fullLayout);
gd._fullLayout.shapes[index] = options;

var attrs = {
'data-index': String(index),
Expand All @@ -273,15 +292,18 @@ shapes.draw = function(gd, index, opt, value) {

var lineColor = options.line.width ? options.line.color : 'rgba(0,0,0,0)';

var path = fullLayout._shapelayer.append('path')
var path = gd._fullLayout._shapelayer.append('path')
.attr(attrs)
.style('opacity', options.opacity)
.call(Plotly.Color.stroke, lineColor)
.call(Plotly.Color.fill, options.fillcolor)
.call(Plotly.Drawing.dashLine, options.line.dash, options.line.width);

if(clipAxes) path.call(Plotly.Drawing.setClipUrl, 'clip' + fullLayout._uid + clipAxes);
};
if(clipAxes) {
path.call(Plotly.Drawing.setClipUrl,
'clip' + gd._fullLayout._uid + clipAxes);
}
}

function decodeDate(convertToPx) {
return function(v) { return convertToPx(v.replace('_', ' ')); };
Expand Down
76 changes: 62 additions & 14 deletions test/jasmine/tests/shapes_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');


describe('Test shapes nodes', function() {
describe('Test shapes:', function() {
'use strict';

var mock = require('@mocks/shapes.json');
Expand All @@ -28,26 +28,74 @@ describe('Test shapes nodes', function() {
return d3.selectAll('.shapelayer').size();
}

function countPaths() {
function countShapePaths() {
return d3.selectAll('.shapelayer > path').size();
}

it('has one *shapelayer* node', function() {
expect(countShapeLayers()).toEqual(1);
});
describe('DOM', function() {
it('has one *shapelayer* node', function() {
expect(countShapeLayers()).toEqual(1);
});

it('has as many *path* nodes as there are shapes', function() {
expect(countShapePaths()).toEqual(mock.layout.shapes.length);
});

it('has as many *path* nodes as there are shapes', function() {
expect(countPaths()).toEqual(mock.layout.shapes.length);
it('should be able to get relayout', function(done) {
expect(countShapeLayers()).toEqual(1);
expect(countShapePaths()).toEqual(mock.layout.shapes.length);

Plotly.relayout(gd, {height: 200, width: 400}).then(function() {
expect(countShapeLayers()).toEqual(1);
expect(countShapePaths()).toEqual(mock.layout.shapes.length);
}).then(done);
});
});

it('should be able to get relayout', function(done) {
expect(countShapeLayers()).toEqual(1);
expect(countPaths()).toEqual(mock.layout.shapes.length);
function countShapes(gd) {
return gd.layout.shapes ?
gd.layout.shapes.length :
0;
}

function getLastShape(gd) {
return gd.layout.shapes ?
gd.layout.shapes[gd.layout.shapes.length - 1] :
null;
}

Plotly.relayout(gd, {height: 200, width: 400}).then(function() {
expect(countShapeLayers()).toEqual(1);
expect(countPaths()).toEqual(mock.layout.shapes.length);
done();
function getRandomShape() {
return {
x0: Math.random(),
y0: Math.random(),
x1: Math.random(),
y1: Math.random()
};
}

describe('Plotly.relayout', function() {
it('should be able to add a shape', function(done) {
var index = countShapes(gd);
var shape = getRandomShape();

Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() {
expect(getLastShape(gd)).toEqual(shape);
expect(countShapes(gd)).toEqual(index + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

@n-riesco nice test. Thanks.

Could you also add an expect(countShapeLayers()) to make sure that the shapes are drawn correctly on relayout?

}).then(done);
});

it('should be able to remove a shape', function(done) {
var index = countShapes(gd);
var shape = getRandomShape();

Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() {
expect(getLastShape(gd)).toEqual(shape);
expect(countShapes(gd)).toEqual(index + 1);
}).then(function() {
Plotly.relayout(gd, 'shapes[' + index + ']', 'remove');
}).then(function() {
expect(countShapes(gd)).toEqual(index);
}).then(done);
});
});
});