Adds remove method to layer (#63).

[samselikoff]

First-pass implementation. Seems to work (I removed the .remove call in the bar chart example, and the bars were removed), but obviously I need to add some tests.

Not super familiar with Sinon. I tried to spy on the layer's remove method, but I couldn't get it working.

[jugglinmike]

You'll be hard-pressed to access the default internal remove method from the tests. That's okay, though, because (for the default implementation) what you really want to do is ensure that the element is removed from the document (not that any particular JavaScript function was invoked)

[jugglinmike]

Thanks for putting this together, @samselikoff . I made a few comments in-line, but there's also one piece of functionality that's missing:

The remove function should be configurable. This will require a change in ['layer-extensions.js](https://github.com/misoproject/d3.chart/blob/27e2224fac8259cb9cf6a9f6857bd4b6ad2068a5/src/layer-extensions.js). Unlike dataBindandinsert, though, this function is optional, so you'll want to check options.removebefore attaching it to the locallayer` instance.

[samselikoff]

Is there a more elegant way to get this? this.layer.selectAll('g') will return a length-1 array even in after the elements have been removed, since it's the selection length rather than the length of the actual DOM elements.

[jugglinmike]

You'll want to use d3's size method. You'll find the API documentation for that here

[samselikoff]

Updated - sorry about the delay!

[samselikoff]

Just saw your comment above - will add this.

[samselikoff]

@jugglinmike ready for review

[jugglinmike]

This test description is a little inaccurate now since in this case, there is no provided remove method.

[jugglinmike]

Thanks, @samselikoff. No worries about the delay: I know it can be tough to make time for this stuff!

[samselikoff]

@jugglinmike I just checked if remove was called, but didn't know how to get the correct context.

[jugglinmike]

We've stubbed the exit selection for just this reason. You can take a cue from this lifecycle event test for an idea on how to access it.

As it stands, this test demonstrates a bug in your implementation: the exit method should not be invoked if the selection is empty. You may remember identifying this in the other lifecycle events over in gh-69.

[samselikoff]

Alright, I think I've got the context correct now.

I made sure exit wasn't called for an empty selection even if remove is provided, so I think I'm missing something about your second point. Are you talking about remove itself? It shouldn't be called if the exit selection is empty?

[jugglinmike]

Hey @samselikoff!

I'm sorry for leaving you hanging in the review process. Regarding the last
change for this patch, I don't think I did a very good job explaining it in
words. Here's a code example that demonstrates what I had in mind:

diff --git a/src/layer.js b/src/layer.js
index f3b6823..b2a56ca 100644
--- a/src/layer.js
+++ b/src/layer.js
@@ -229,5 +229,7 @@ Layer.prototype.draw = function(data) {
        }
      }

-     this.remove.call(selection);
+     if (!selection.empty()) {
+       this.remove.call(selection);
+     }
 };
diff --git a/test/tests/layer.js b/test/tests/layer.js
index e0686ae..0bc033c 100644
--- a/test/tests/layer.js
+++ b/test/tests/layer.js
@@ -106,6 +106,12 @@ suite("d3.layer", function() {

          assert(this.remove.calledOn(exiting));
        });
+       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() {

besides that, there's just a trivial issue with white space to clean up (we're
using tabs in this project--I'll set us up with JSCS shortly so that this is
more obvious).

It's been quite a while since we worked on this, though, and I imagine your
interests have drifted. I'm more than happy to make these changes on top of
your patch and land it that way, but I wanted to give you an opportunity to
finalize this in the unlikely event that you're not completely exhasperated.

Let me know either way!

[samselikoff]

I appreciate the follow-up - I'd need to spend some more time with this to re-familiarize myself and have a few other projects I'm focusing on now.

Again, I really appreciate your feedback!