Implement Layer#remove
#109
Conversation
Do not invoke a layer instance's `remove` handler if the `exit` selection is empty. This completes the implementation of `Layer#remove`.
|
Ah was confused by the title of this PR. So this is just adding the ability to override the behavior of |
|
This patch:
|
|
@iros Would you mind taking a look? |
| @@ -216,4 +228,8 @@ Layer.prototype.draw = function(data) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| if (!selection.empty()) { | |||
There was a problem hiding this comment.
What's the reasoning behind calling remove on the selection if it's not empty? I might be missing some context here.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think there's a crossed wire here. Let's discuss this in person.
|
Added some questions @jugglinmike. |
|
Added some responses, @iros :P |
|
I responded to your responses @jugglinmike. 🍌 ➡️ 📫 = 🎉 |
This configurable hook was initially designed in gh-63, and the implementation was proposed in gh-103. As discussed there, I'm following up with a few minor changes to finalize the implementation. Thanks for the help, @samselikoff!
Resolves gh-63