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.

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

Why have an insert method? #63

Open
samselikoff opened this issue Dec 9, 2013 · 10 comments
Open

Why have an insert method? #63

samselikoff opened this issue Dec 9, 2013 · 10 comments

Comments

@samselikoff
Copy link

Hello,

I'm trying to wrap my head around the insert method. In the world of vanilla D3, we typically append elements within the enter selection:

d3.select("body").selectAll("p")
    .data([4, 8, 15, 16, 23, 42])
  .enter().append("p")
    .text(function(d) { return "I’m number " + d + "!"; });

In d3.chart, we'd do something like this:

d3.chart('PChart', g, {
  ...
  insert: function() {
    return this.append('p');
  },

  events: {
    'enter': function() {
      this.text(function(d) { return "I’m number " + d + "!"; });
    }
  }
});

My question is, why not just append the elements in the enter lifecycle event? I may not be reading it right, but isn't this what the relevant code is essentially doing?

The only info I've been able to find about this was in Irene's blog post:

insert method - the method that creates the actual data-bound elements and sets any attributes that don’t have to do with the data. The reason we don’t do that here is because we want to account for our data potentially changing and thus want to delegate setting any data-driven attributes to the lifecycle events.

But isn't appending elements a data-driven operation, and therefore doesn't it belong in the enter lifecycle event?

I think I'm just missing something here - would love some clarification.

Thanks!

@jugglinmike
Copy link
Member

Hi Sam,

We do this so that the context of the enter event is consistent with the other lifecycle event handlers--it is a d3 selection of the elements described by the event. In the source code you've linked to, you'll note that the "selection" for the enter event is actually stored as a bound function. We do this so we may defer the insertion operation until after the update event and so that when we have inserted the new element(s), we can use the resulting selection as the context for the related lifecycle event handlers.

More than just being a technical requirement, we think this also promotes chart legibility. The logic that dictates how elements are created is fundamental to the chart's definition, so we want that encapsulated in an explicit function--not declared within one (of many) lifecycle event handlers.

Does that clear things up?

@samselikoff
Copy link
Author

Hmm, the code is a bit over my head. I can't quit understand that point.

Couldn't you defer the insertion operation simply by invoking the enter handler after the update handler?

Perhaps it would help me understand if you explained why we wouldn't do the same for removing elements. That is, why don't we have an explicit function for remove which is invoked after the exit handler is invoked, and is responsible for removing elements from the DOM? To me that seems similar to what we're doing here, yet we don't do that with d3.chart - we remove elements in the exit selection.

Thanks for your time + quick response Mike!

@jugglinmike
Copy link
Member

Couldn't you defer the insertion operation simply by invoking the enter handler after the update handler?

Actually, we could, but modifying the selection in place means that each handler could be dealing with a different set of elements. If we promote a pattern where any lifecycle event handler may add elements to the set, the value of this across handlers could mean different things for the same event. I think this could be confusing; do you agree?

why don't we have an explicit function for remove which is invoked after the exit handler is invoked, and is responsible for removing elements from the DOM?

I think we document removing nodes in in exit because it typically happens at the end of a transition, so the selection is valid for others to attach to. That said, this does make the API a bit inconsistent (especially given my above explanation), and your recommendation sounds like a good idea. Thanks!

@samselikoff
Copy link
Author

Ah, I think I'm starting to understand. I haven't worked with complex enough extensions where I've written multiple handlers for the same event. You're saying that if one of those handlers appended some elements, it would change the selection for the other handlers - right? Avoiding that situation makes a lot of sense.

While I have you - how exactly does append change the selection? Have you found a good explanation anywhere?

@samselikoff
Copy link
Author

To clarify - are you saying that even if you remove() elements in one exit handler, the selection will still be there in a second exit handler?

@jugglinmike
Copy link
Member

Hey @samselikoff,

I think this is good justification for a dedicated remove function. A default implementation like

function() {
  this.remove();
}

would seem to be sensible and shouldn't cause too much confusion when people migrate to the next release. If you'd like to give it a shot, I'd be more than happy to help you implement this feature. Otherwise, just let me know and I'll give it a go myself.

@samselikoff
Copy link
Author

I'd love to try and implement this, and really appreciate your help!

So by default, the exit selection will get .remove()'d, and if a user writes their own remove handler, it will get used instead?

@jugglinmike
Copy link
Member

Yup, that sounds about right

@samselikoff
Copy link
Author

@jugglinmike it would need to go after the events loop, so exit:transition handlers are invoked, correct?

I don't fully grok this code yet, I was expecting insert to be separate from enter but I think it's picked up by these lines. Could you maybe shed some light?

@jugglinmike
Copy link
Member

You have the right idea. The insert function is what creates the enter lifecycle selection; that's why they're coupled together. Since the exit selection is the last to be made in the loop, it should probably be alright to insert the remove invocation after the loop terminates.

samselikoff added a commit to samselikoff/d3.chart that referenced this issue Jun 28, 2014
jugglinmike pushed a commit to jugglinmike/d3.chart that referenced this issue Jun 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants