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

Pass DOM element to selection functions #2246

Closed
mminer opened this issue Feb 27, 2015 · 8 comments
Closed

Pass DOM element to selection functions #2246

mminer opened this issue Feb 27, 2015 · 8 comments
Milestone

Comments

@mminer
Copy link

mminer commented Feb 27, 2015

Currently the context of a function passed to selection.attr (and its sibling operations) is set to the DOM element of the selection. Things get tricky when you want to both use the outer this and do something with the DOM element. Of course, a var self = this; before the function solves this.

var self = this;

d3.select('#some-element')
    // Completely contrived example of using both this and self:
    .attr('class', function() {
        return (self.highlightA && this.dataset.a) ? 'highlight' : 'normal';
    });

What would be more convenient is if the DOM element was available as a parameter to the function, i.e. (d, i, element). This would allow you to use ES6's arrow functions, which ignore the function's context.

d3.select('#some-element')
    // Completely contrived example of using both this and self:
    .attr('class', (d, i, elem) => (this.highlightA && elem.dataset.a) ? 'highlight' : 'normal');

To be clear, I'm not proposing that the context be changed to something else, only that it be possible to get the relevant DOM element without it.

Regarding backwards compatibility, adding an additional argument shouldn't break anyone's code, unless they're for some reason relying on arguments being a particular length (though if my assumption is mistaken and such a change would indeed be a breaking one, it should probably be left until version 4).

@jasondavies
Copy link
Contributor

Related: ES6’s fat arrow syntax uses a lexical binding for this. In other words, you cannot access the this context that D3 passes to callbacks if you’re using the fat arrow syntax. The solution: don’t use the fat arrow syntax, or alternatively, we may need to consider something like the suggestion above for version 4.0.

@jasondavies jasondavies added this to the 4.0 milestone Mar 24, 2015
@mgold
Copy link
Contributor

mgold commented Mar 24, 2015

@mminer Part of what makes this tricky is that functions are actually passed three arguments (not counting this); the third is the index in the current group. An expert feature, but sometimes an indispensable one. I'd hate to throw a DOM element between these indices.

@mminer
Copy link
Author

mminer commented Mar 24, 2015

That's understandable. I don't suppose that adding it as the fourth argument would be acceptable? I recognize at this point though that it gets into argument-overload territory.

@mgold
Copy link
Contributor

mgold commented Mar 26, 2015

If you add it as the fourth argument, then everyone has to know about j (and write it in the argument list) . I won't deny that this can be a headscratcher, but it's hardly the only JavaScript "feature" that D3 has turned to its advantage.

@IPWright83
Copy link

@mgold I'd suggest pushing the 3rd argument (index in current group) back to be the 4th parameter, inserting the DOM element in between. Ordinarily I wouldn't advocate such a breaking move, but as it's not mentioned in the API docs it's likely to be an infrequently used feature on the whole.

The sort of code that I'd like to be writing is similar (not using arrow functions) like:

d3,selectAll("circle")
    .on("click", this.click.bind(this));

this.click = function(d, i, domElement) {
   // Now I can call this.someFunc as this has been preserved
   this.someExtraFunc();
}

@chrisprice
Copy link

Would an alternative be to use (abuse?) another JavaScript feature and make the node available as d3.node within the function?

  • + Allows use with ES6 arrow functions.
  • + Follows an existing convention (d3.event).
  • + Is a bit of an edge case (in the same way as d3.event) and so a slightly unusual syntax is more acceptable.
  • + Is compatible with the new parent data changes.
  • + Is backwards compatible.
  • - Looks odd.

@mbostock
Copy link
Member

Our options are:

  1. Pass the node as a new argument at the beginning of the list.
  2. Pass the node as a new argument at the end of the list.
  3. Temporarily set a global while the function is being evaluated.

Passing the selected node at the beginning is bad because there are many times where you don’t need the selected node. This is especially true when you want to pass in another function that operates on your data. For example, if your data is numbers, you might apply a scale to set an attribute like so:

d3.selectAll("circle")
    .data([0, 1, 2])
    .attr("cx", d3.scale.linear()
        .domain([0, 10])
        .range([0, width]));

If the first argument is now the selected node, you’d have to say:

var x = d3.scale.linear()
    .domain([0, 10])
    .range([0, width]);

d3.selectAll("circle")
    .data([0, 1, 2])
    .attr("cx", function(node, d) { return x(d); }); // Blech!

This pattern of passing a function to a selection method is also common with formatting numbers.

Passing the selected node at the end also doesn’t work very well because it requires you to know how many arguments are being passed to your function. In D3 3.x, that is typically three values: d, the datum; i, the index; and j, the group index. (See How Selections Work and Nested Selections.) But people might assume only d and i, since those are used most commonly. Also there a handful of methods that don’t pass the group index (#2413)…

In D3 4.0, the call pattern will likely change slightly. Selections in 4.0 are arbitrary hierarchies rather than limited to one level of grouping: the arguments for a flat selection are d and i; the arguments for a singly-grouped selection are d, i, p and j, where p is the parent datum; for further grouping, there are correspondingly more arguments. Thus it’s awkward to access the selected node as the last argument because the position depends on the grouping. And technically, you can already access the selected node without this via the indexes, but it’s similarly awkward and you must use the semi-private _root field:

selection.each(function(d, i, p, j) {
  this === selection._root[j][i]; // true!
});

Setting the selected node as a global (d3.node) is also a bit icky, but as @chrisprice points out it’s similar to how we set d3.event during an event handler. @wil93’s example would look like this:

d3.selectAll(".region").each(d => {
  d3.select(d3.node).style("fill", this.computeColor());
});

A variation on this idea is a new d3.activeSelection (or d3.selectActive?) method, in the same vein as d3.activeTransition as described in d3/d3-transition#15. It is similar to d3.select(d3.node), although I expect it should retain the same level of grouping as the context selection. For example:

d3.selectAll(".region").each(d => {
  d3.activeSelection().style("fill", this.computeColor());
});

(Though, note that d3.activeTransition as proposed in the other issue takes a node as argument; that would presumably go away if it could infer the active element automatically.)

If we go the global route, we might also take that approach for setting the current index, and then remove those pesky i and j arguments. That has some nice properties, and is essentially the same pattern that Protovis used. Or maybe we can abandon the indexes entirely now? Edit 1: I don’t think we can abandon indexes entirely because they are very convenient as implicit data. Edit 2: Also, a number of non-selection functions follow the similar pattern of d and i, such as d3-arrays’ min and max, and d3-shape’s line.x and line.y. Though I wonder how we would represent the parent node and parent indices if we go the global route.

@mbostock
Copy link
Member

mbostock commented Mar 1, 2016

I’ve standardized d3-selection and d3-transition on the current datum d, the group index i, and the group nodes array. This is intended to mimic array methods such as array.forEach. The this is the current element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants