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

Multi-value map support. #277

Closed
wants to merge 10 commits into from
Closed

Multi-value map support. #277

wants to merge 10 commits into from

Conversation

mbostock
Copy link
Member

Current progress:

  • selection.attr - done
  • selection.style - done
  • selection.property - done
  • selection.classed - done
  • selection.on - done
  • transition.attr - done
  • transition.style - done

Also, we may want to include these, but I'm not 100% yet:

  • transition.attrTween?
  • transition.styleTween?
  • transition.tween?

@mbostock
Copy link
Member Author

There's probably a way to structure the code into two methods (a getter and a setter) per operator, and then write a generic wrapper that detects multi-valued maps and dispatches accordingly.

@syntagmatic
Copy link

If transition.style was implemented without transition.styleTween-- does that mean all style transitions with a multi-value would happen instantaneously?

@mbostock
Copy link
Member Author

No, that's not what I meant. I meant that we could add multi-value map support for transition.style, but potentially transition.styleTween would still only support single values. Though it would be preferable for it to support a map for consistency with other operators.

@mhalle
Copy link

mhalle commented Nov 5, 2011

Would it be possible to do this merge at this time?

@mbostock
Copy link
Member Author

mbostock commented Nov 5, 2011

Nope; still need to add support for transition.attr and transition.style, at the least. I might be okay deferring selection.classed and selection.on to a later release. Feel free to lend a hand if you want to see this happen more quickly!

@jasondavies
Copy link
Contributor

I did some work on this about a month ago: https://github.com/jasondavies/d3/tree/map. In @ba2b1537a18762d8ef4147c27ef3104d622ddc2e I didn't get round to adding tests, that's the next step for transition.attr. Almost there!

@dandean
Copy link

dandean commented Mar 15, 2012

Any idea if this will be merged in?

I'm working on my own much simpler attrs implementation which basically iterates a map and calls attr. But, if there are plans to merge this pull request in, I'd love to take advantage of its enhanced functionality.

Here's my branch, for reference:

https://github.com/dandean/d3/commit/a0754f6da6e035c7eb804253b42579713703a7d1

@mbostock
Copy link
Member Author

Shooting for the next minor release, 2.9. Hopefully that should be within the next couple of weeks.

@dandean
Copy link

dandean commented Mar 15, 2012

Cool - thanks @mbostock !

@mattsacks
Copy link

👍

This would be great to see in 2.9

@mbostock
Copy link
Member Author

mbostock commented Aug 5, 2012

Related #55 #65

@mbostock
Copy link
Member Author

mbostock commented Aug 5, 2012

So far, I have added support for accepting both a map and a function that returns a map. When a map is specified (say to selection.attr), it can contain constants or functions. However if a function is specified, the map it returns can only specify constants. I was striving for consistency with other methods that allow either constants or functions, but I feel like this adds too many equivalent signatures and fails at parsimony. For example, these are all equivalent:

selection.attr("foo", 42); // constant
selection.attr("foo", function() { return 42; }); // function
selection.attr({foo: 42}); // map of constants
selection.attr({foo: function() { return 42; }}); // map of functions
selection.attr(function() { return {foo: 42}; }); // function returning a map of constants

In the earlier discussion for #55 and #65, it seemed like the last variant is probably overkill. It’s arguably the most powerful of the five signatures because you can share work across multiple attributes and compute attribute names dynamically. However, you can already structure your code this way using selection.each, though perhaps with a bit more typing and perhaps slightly worse performance:

selection.each(function() { d3.select(this).attr("foo", 42); });

Also consider this implies three signatures for selection.on:

selection.on("click", listener); // listener
selection.on({click: listener}); // map of listeners
selection.on(function() { return {click: listener}; }); // function returning a map of listeners

I’d like to kill support for the single-function signatures. That would leave four signatures for selection.attr (ignoring the fifth signature when used as a getter):

selection.attr("foo", 42); // constant
selection.attr("foo", function() { return 42; }); // function
selection.attr({foo: 42}); // map of constants
selection.attr({foo: function() { return 42; }}); // map of functions

And two for selection.on:

selection.on("click", listener); // listener
selection.on({click: listener}); // map of listeners

This preserves D3’s philosophy of encouraging only values (and not names) to be specified as functions.

@syntagmatic
Copy link

I initially preferred the last variant (function returning a map of constants) over the second-to-last (map of functions), but after thinking on it for a few minutes I think this is a good compromise.

Using selection.each initially seems like a kludge, but within that function you can construct a map of constants for not only .attr(), but also .style(), .on(), etc. It would be quite verbose though.

The function returning a map of constants does look very pretty in coffeescript.

selection.attr (d) ->
  cx: x(d)
  cy: y(d)
  fill: color(d)
selection.attr(function(d) {
  return {
    cx: x(d),
    cy: y(d),
    fill: color(d)
  };
});

vs the map of functions

selection.attr
  cx: (d) -> x(d)
  cy: (d) -> y(d)
  fill: (d) -> color(d)
selection.attr({
  cx: function(d) { return x(d); },
  cy: function(d) { return y(d); },
  fill: function(d) { return color(d); }
});

@mbostock
Copy link
Member Author

mbostock commented Aug 5, 2012

A small point, but you can collapse the map of functions slightly in your example:

selection.attr({
  cx: x,
  cy: y,
  fill: color
});

Or in CoffeeScript:

selection.attr
  cx: x
  cy: y
  fill: color

This only works when the functions x, y and color take the standard signature (d, i). You wouldn’t derive this advantage from the function that returns a map, since you have to invoke the functions manually.

@syntagmatic
Copy link

Ahh, very concise!

@idibidiart
Copy link

selection.attr({
cx: x,
cy: y,
fill: color
});

For the new user, reading the above, it's hard to tell that x, y and color are in-line declared functions because it's more typical to see a function declaration in this type of situation or else the assumption is that it's a function expression defined elsewhere or just a variable. Ugh, a little mind bending but any say I can't say I don't love it..

@idibidiart
Copy link

in-line declared was meant to read: implicitly declared

@mbostock
Copy link
Member Author

mbostock commented Aug 6, 2012

@idibidiart True, but that also applies to the previous syntax:

selection
    .attr("cx", x)
    .attr("cy", y)
    .attr("fill", color);

@idibidiart
Copy link

@mbostock

I didn't know that because I didn't expect it

The more I get into D3 the more my mind opens up to Design As Magic and further away from Design As Commodity

Not to be dramatic or anything but it certainly appeals to the magician/mathematician in me (in terms of its style) more than the typical "end user" The Principle Of Least Surprise appears far less important in the design of D3 than its non-trivial, almost magical design patterns :)

Sorry to go off topic. I just wanted to relay my subjective experience of the medium (D3) as I'm getting to know it

:)

Marc F

@mbostock
Copy link
Member Author

mbostock commented Aug 9, 2012

Merged into #756; staged for 2.10.0.

@mbostock mbostock closed this Aug 9, 2012
@dribnet
Copy link

dribnet commented Feb 27, 2013

I’d like to kill support for the single-function signatures.

😢

I get it.

Nevertheless replacing wasteful code like:

      .enter().append("line")
        .attr("x1", function(d) { var p = project([d.lon, d.lat]); return p[0]; })
        .attr("y1", function(d) { var p = project([d.lon, d.lat]); return p[1]; })
        .attr("x2", function(d) { var p = project([d.lon, d.lat]); return p[0]+10; })
        .attr("y2", function(d) { var p = project([d.lon, d.lat]); return p[1]+10; });

with

      .enter().append("line")
        .each(function(d) {
            var p = project([d.lon, d.lat]);
            d3.select(this)
                .attr({ x1:p[0], y1:p[1], x2:(p[0]+10) y2:(p[1]+10) });
        });

instead of

      .enter().append("line")
        .attr(function(d) { 
            var p = project([d.lon, d.lat]);
            return { x1:p[0], y1:p[1], x2:(p[0]+10) y2:(p[1]+10) }; 
        });

makes me shed one tear... but maybe combinatorial explosion of method signatures would have made me shed two.

@coli
Copy link

coli commented May 6, 2015

I really wish this is supported..

selection.attr(function() { return {foo: 42}; });

@mbostock
Copy link
Member Author

mbostock commented May 6, 2015

@coli This is discussed in #2109, and I think we can do this for 4.0 as selection.attrs (plural).

@coli
Copy link

coli commented May 6, 2015

Thanks!

Using each + select as a workaround just turns functional code back to being procedural code again.

Edit: it breaks animation too.

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

Successfully merging this pull request may close these issues.

9 participants