Skip to content
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

Jump to bottom

Implement Layer#remove #109

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement Layer#remove #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1437398
Add remove method to layer #gh-63
samselikoff Jun 28, 2014
205178e
Correct code style inconsistencies
jugglinmike Jun 28, 2015
3f3b0f3
Limit invocation of `remove` handler
jugglinmike Jun 18, 2015
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
3 changes: 1 addition & 2 deletions examples/scripts/bar-chart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ d3.chart("BarChart", {

function onExitTrans() {
this.duration(1000)
.attr("x", function(d, i) { return chart.x(i - 1) - .5; })
.remove();
.attr("x", function(d, i) { return chart.x(i - 1) - .5; });
}

function dataBind(data) {
Expand Down
5 changes: 4 additions & 1 deletion src/layer-extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@ d3.selection.prototype.layer = function(options) {
var layer = new Layer(this);
var eventName;

// Set layer methods (required)
// Set layer methods (databind/insert required, remove optional)
layer.dataBind = options.dataBind;
layer.insert = options.insert;
if (options.remove) {
layer.remove = options.remove;
}

// Bind events (optional)
if ("events" in options) {
Expand Down
22 changes: 19 additions & 3 deletions src/layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ Layer.prototype.insert = function() {
d3cAssert(false, "Layers must specify an `insert` method.");
};

/**
* Invoked by {@link Layer#draw} in order to remove exiting DOM nodes from
* this layer's `base`. This default implementation may be overridden by
* Layer instances.
*/
Layer.prototype.remove = function() {
this.remove();
};

/**
* Subscribe a handler to a "lifecycle event". These events (and only these
* events) are triggered when {@link Layer#draw} is invoked--see that method
Expand Down Expand Up @@ -113,9 +122,12 @@ Layer.prototype.off = function(eventName, handler) {

/**
* Render the layer according to the input data: Bind the data to the layer
* (according to {@link Layer#dataBind}, insert new elements (according to
* {@link Layer#insert}, make lifecycle selections, and invoke all relevant
* handlers (as attached via {@link Layer#on}) with the lifecycle selections.
* (according to {@link Layer#dataBind}), insert new elements (according to
* {@link Layer#insert}), make lifecycle selections, invoke all relevant
* handlers (as attached via {@link Layer#on}) with the lifecycle selections,
* then remove exiting elements (according to {@link Layer#remove}).
*
* The lifecycle selections are:
*
* - update
* - update:transition
Expand Down Expand Up @@ -216,4 +228,8 @@ Layer.prototype.draw = function(data) {
}
}
}

if (!selection.empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind calling remove on the selection if it's not empty? I might be missing some context here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for consistency with how we invoke the lifecycle events--see this logic in Layer#draw.

...but it's not really fair to answer a question like, "Why are we doing this here?" with, "because we're doing it over there." The underlying motivation was originally discussed in this issue: #69 .

Does this seem right to you?

Copy link
Member

Choose a reason for hiding this comment

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

So I think the way I would have approached it, is by doing the selection check here:
https://github.com/jugglinmike/d3.chart/blob/gh-103-extensions/src/layer.js#L217
and here:
https://github.com/jugglinmike/d3.chart/blob/gh-103-extensions/src/layer.js#L227

What I worry about is the remove routine having more destructive behavior (like removing parent containers that the selection would go into) that you might not want to execute unless you're effectively 'done' redrawing this layer or done with the chart itself (or perhaps you want to hide it somehow.)

Is there a reason we couldn't localize it to the specific lifecycle events and just not call the handlers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's a crossed wire here. Let's discuss this in person.

this.remove.call(selection);
}
};
38 changes: 37 additions & 1 deletion test/tests/layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,19 @@ suite("d3.layer", function() {
sinon.spy(entering, "transition");
return entering;
});
var remove = this.remove = sinon.spy();
var base = this.base = d3.select("#test").append("svg");

this.layer = base.layer({
dataBind: dataBind,
insert: insert
});

this.layerWithRemove = this.base.append("g").layer({
dataBind: dataBind,
insert: insert,
remove: remove
});
});

test("invokes the provided `dataBind` method exactly once", function() {
Expand Down Expand Up @@ -82,7 +89,30 @@ suite("d3.layer", function() {
this.layer.draw([]);
assert(this.insert.calledOn(this.dataBind.returnValues[0].enter.returnValues[0]));
});

test("by default removes exiting nodes from the DOM", function() {
this.layer.draw([1]);
assert.equal(this.layer.selectAll("g").size(), 1);
this.layer.draw([]);
assert.equal(this.layer.selectAll("g").size(), 0);
});
test("invokes the provided `remove` method in the context of the layer's bound 'exit' selection", function() {
var updating, exiting;

this.layerWithRemove.draw([1, 2, 3]);
this.layerWithRemove.draw([]);

updating = this.dataBind.returnValues[1];
exiting = updating.exit.returnValues[0];

assert(this.remove.calledOn(exiting));
Copy link
Member

Choose a reason for hiding this comment

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

So, I think it's premature to assume that drawing an empty set is equivalent to removing the layer. I can see, for example, visualizing streaming data might result in gaps that would just not have any nodes in the rendering cycle for that subset, but that might change come next data frame. I think this also adds a bit of unstated "magic" whereby we're saying calling draw with an empty set will result in the layer removal being called. I would prefer that were a more explicit user choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the issue that prompted this change (#63), we identified an asymmetry between how data-bound elements are inserted and how they are removed. In the former case, d3.chart requires a function dedicated to the task of insertion. In the latter case, users are expected to remove data-bound elements in a lifecycle event. Layer#remove is an attempt to formalize the way charts define "cleanup" logic for data-bound elements. It does not remove the layer wholesale, though--this is distinct from d3.selection.prototype.remove.

In light of that, I don't believe this patch suffers from the problem you are describing here. Can you confirm? If so, I think this is indicative of a confusing method name, and we should talk about finding a better name!

Copy link
Member

Choose a reason for hiding this comment

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

So is the assumption that remove will be defined just like insert and dataBind if a user wishes to do so? I can't think of a scenario where we would need a separate method to basically call .remove on a selection. I tend to call remove in my exit transitions - my worry is that by defining remove as a separate function, it will not be clear that it only pertains to the exiting selection. My initial assumption, that this function enabled one to effectively remove the drawn layer, is probably something that a lot of people will consider it to mean as well.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is definitely indicative of a naming issue. As proposed, "remove" describes functionality for each data-bound element individually when its associated data point is removed from the set. I can see how its generic name and association with the Layer instance itself suggests that the Layer instance is the target.

Unfortunately, a consistent solution will involve a breaking API change. This is because "insert" already suffers from the problem you are identifying (in the same way, it is not the insertion logic for the Layer instance, but the insertion logic for each new data-bound element).

What we would need is some clear, concise nomenclature for "data-bound element". I'm going to use the word "point" for demonstration purposes (even though I admit that is a terrible choice).

The two methods might be better named insertPoint and removePoint to more clearly describe their targets.

d3.select('svg').append('g').layer({
  dataBind: myDataBindFunction,
  insertPoint: myFunctionForInsertingASingleNewDataBoundElement,
  removePoint: myFunctionForRemovingASingleDataBoundElement
});

If we found a better name than "point", do you think this would clear things up?

});
test("does not invoke `remove` method when `exit` selection is empty", function() {
this.layerWithRemove.draw([1, 2, 3]);
this.layerWithRemove.draw([1, 2, 3]);

assert.equal(this.remove.callCount, 0);
});

suite("event triggering", function() {
test("invokes event handlers with the correct selection", function() {
var layer = this.base.append("g").layer({
Expand Down Expand Up @@ -137,19 +167,25 @@ suite("d3.layer", function() {
},
insert: function() {
return this.append("g");
},
remove: function() {
return this.remove();
}
});
var enterSpy = sinon.spy();
var updateSpy = sinon.spy();
var exitSpy = sinon.spy();
layer.draw([1]);

layer.on("enter", enterSpy);
layer.on("update", updateSpy);
layer.on("exit", exitSpy);

layer.draw([1]);

sinon.assert.callCount(enterSpy, 0);
sinon.assert.callCount(updateSpy, 1);
sinon.assert.callCount(exitSpy, 0);
});

suite("Layer#off", function() {
Expand Down