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

.attr( map ) and .attr( function() {return map} ) #179

Closed
wants to merge 6 commits into from
Closed

.attr( map ) and .attr( function() {return map} ) #179

wants to merge 6 commits into from

Conversation

syntagmatic
Copy link

attr can now take a map of name/value pairs, by calling itself recursively for each pair.
attr can now take a function which returns a map.

@mbostock
Copy link
Member

mbostock commented Jun 7, 2011

See related discussions in #55 and #65. The key comment: "That suggests we could support taking a map for attr, style and on—and defer supporting a function that returns a map until a future release (if at all)."

So, yeah. I'm onboard with this pull request, and I'd be happy to include it in the next release. I think we should extend this syntax to style and on. Technically, it'd also be good to return a map if these methods are called with no arguments, but we could potentially defer that to a future release, along with the function form (for generating the map dynamically per element).

@jasondavies
Copy link
Contributor

Sounds great. I'm quite interested in the function form too, as I think it could simplify code that needs to switch between orientations.

@syntagmatic
Copy link
Author

Agree style and on would benefit from this syntax. I'll implement something similar to jQuery's access function, which abstracts the overloading to a separate function and optionally executes values which are functions.

@syntagmatic
Copy link
Author

This change has been applied to style, on, and property. Those functions also return this if an object is passed in, so they can be chained (previous version broke this).

You can check out the new syntax in chord.js

I interpreted the "function form" you both mentioned as passing in anonymous functions as values in the map, but delaying their execution until mapped to data. That should be the current behavior. Is this correct?

Optional execution of function values was not implemented. A self-executing function, function () {}() can be used for this purpose.

There is a new function, d3.each, which is a general-purpose each for objects (but not lists) which can take a single extra argument.

@mbostock
Copy link
Member

mbostock commented Jun 8, 2011

I interpreted the "function form" you both mentioned as passing in anonymous functions as values in the map, but delaying their execution until mapped to data. That should be the current behavior. Is this correct?

No, the inverse of that, which is "a function that returns a map". For example:

    .attr(function() { return {rand: Math.random()}; })

It's possible to support both, though I'm a little worried about the complexity and performance. But being similar to jQuery is also valuable. I'll take a look at your implementation and see if I have any suggestions. Thanks!

@syntagmatic
Copy link
Author

Added basic support for the function form .attr( function() {return map} ), but evaluation of the returned map is fragile.

The issue is attrNull and attrFunction depend on name and value in the attr namespace. I re-implemented part of attrFunction to support functions within the map, such as cx and cy in the following:

.attr( function(d) {
  return {
    "cx": function() { return 100 + Math.random() * 800; },
    "cy": function() { return 100 + Math.random() * 800; },
    "r": 50 * d,
    "fill": "hsl(" + (100*d+100) + ", 70%, 50%)"
  }
});

Find the above code in action on hello sort.

@mbostock
Copy link
Member

mbostock commented Jun 9, 2011

Nice work! I think it's probably overkill to support functions that contain functions, although I appreciate your attention to detail and desire to support consistent behavior. We started with two types of input:

1- A constant, such as 42.
2- A function, such as function(d) { return d * 2; }.

Initially, I was thinking of adding two more types, in the single-argument form of attr:

3- A constant map, such as {cx: 42}.
4- A function that returns a constant map, such as function(d) { return {cx: d * 2}; }.

This latter form should be sufficient for the single-argument form, where you might even want to control dynamically which attributes you specify. Now, I agree there are other possible forms:

  • A map that contains functions, such as {cx: function(d) { return d * 2; }}.
  • A function that returns a map that contains functions.

While these might be expected, for consistency's sake, they don't seem particularly useful given that they can always be expressed by the fourth form above. For the sake of keeping the implementation small and fast, I'd prefer to support as few forms as possible. I'm normally in favor of strict parsimony, so for me the fourth form is the really convincing use case, as it's the one that lets you alter which attributes you specify as a function of data.

@syntagmatic
Copy link
Author

I agree that a function that a returns a map that contains functions isn't useful. My code snippet could be equivalently written:

.attr( function(d) {
  return {
    "cx": 100 + Math.random() * 800,
    "cy": 100 + Math.random() * 800,
    "r": 50 * d,
    "fill": "hsl(" + (100*d+100) + ", 70%, 50%)"
  }
});

What about attrNull. If the function returns a map with null values, should those attributes be removed?

The rest is bikeshedding

I disagree on a map that contains functions, which is cheap in complexity with a recursive call to attr and does improve consistency. Performance cost seems minimal.

It's a more straightforward translation from existing code, especially when only one attribute is a function of d:

// chained attrs
.attr("y", y1)
.attr("height", function(d) { return y0(d) - y1(d); })

// map contains a function (the bikeshed)
.attr({
  "y": y1,
  "height": function(d) { return y0(d) - y1(d); }
});

// function returns a map
.attr(function(d) {
  return {
    "y": y1,
    "height": y0(d) - y1(d)
  };
})

TMTOWTDI. They all look nice to me.

@mbostock
Copy link
Member

mbostock commented Jun 9, 2011

Yep, if you say attr({cx: null}), it should delete the "cx" attribute, as if you said attr("cx", null).

If it's simpler to support a map that contains functions, then I say go for it. :) I'll take a look.

@@ -1395,6 +1403,27 @@ function d3_selection(groups) {
};

groups.attr = function(name, value) {
if (typeof name === "object") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typeof check is a bit ambiguous. For example, today I can say selection.attr(new String("fill"), "white"), and the name argument is automatically coerced from an object to a string. This isn't a particularly strong counterexample, but I prefer to use the arguments.length check for method overloading as the behavior is more predictable.

Ah, wait. That won't work, because we already support a single-argument form for returning the value: selection.attr(name). So now I'm back to thinking we should have a separate attrs method that takes a map so as to avoid any ambiguity. I'm not sure whether it would be more or less confusing to have a different name for the method, though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeof isn't ideal for detecting objects, especially since typeof null === 'object' is true.

Though, jQuery uses this exact check in its access helper function for overloading attr, and I've never hit this issue in practice.

@mbostock
Copy link
Member

mbostock commented Jun 9, 2011

Assuming we have a method attrs(map), here's how it might work. First, define a method that handles the evaluation and application of the map function:

function attrMapFunction() {
  var x = map.apply(this, arguments), name, value;
  for (name in x) {
    value = x[name];
    name = d3.ns.qualify(name);
    value == null
        ? (name.local ? this.removeAttributeNS(name.space, name.local) : this.removeAttribute(name))
        : (name.local ? this.setAttribute(name, value) : this.setAttributeNS(name.space, name.local, value));
  }
}

Then, use a typeof check to branch evaluation for constants or functions:

if (typeof map === "function") {
  groups.each(attrMapFunction);
} else for (var name in map) {
  groups.attr(name, map[name]);
}
return groups;

I think that's pretty small and fast. What do you think?

@mbostock
Copy link
Member

mbostock commented Jun 9, 2011

Oh, it might be cool if calling selection.attrs() returned a map of the defined attributes (mapping namespaces back to prefixes to match the input format)… More work, though. :)

@syntagmatic
Copy link
Author

I'll give the return mapping of defined attributes a shot. attrs will actually return a list of maps in this case? Will this affect function chaining?

I do strongly support overloading attr. I'm attracted to d3 in large part because of its use of SVG attributes and strings. Raphael's attr also uses SVG conventions, and can take a map. The two APIs are so similar, it would be very satisfying to me if they behaved identically when possible.

The function-which-returns-a-map form is a great addition though, since d3's attr always works on collections of data.

@mbostock
Copy link
Member

My implementation branch is available in the map branch, with the main commit being @5603474872ac2cd9abe38b0eceac5e679b5a9080. I haven't tested the performance implications of this change, but it should be negligible.

I decided not to implement the mode where attr() returns a map of all defined attributes. I think it's possible, though a cursory investigation suggested that support for inspecting the list of defined attributes is spotty (however, in browsers that support SVG I suspect it's doable). Moreover, inspecting all attributes at once doesn't seem like an immediate requirement, so I think it's best to punt on that for now and keep the code small.

If you're happy with my implementation, we could work on extending it to style, classed, property and on. In the case of style, we should still allow the priority to be specified, but it would affect all of the entries in the map. We'd also need to support map-style input for transitions: attr, attrTween, style and styleTween. So, while this appears to be a fairly innocuous overloading of one of the operators it's actually a fairly extensive change to the core API. :) Seems like a good change, though!

@syntagmatic
Copy link
Author

The implementation looks great! I'll take a look at how style, etc differ from attr next week. Been learning other parts of the API the past few days.

@syntagmatic
Copy link
Author

What's the status of this update? Has the attr change been merged into the main branch?

@mbostock
Copy link
Member

Not yet. There's a lot more work to do to implement proper map support, including other operators. I've merged master into my map branch if you want to keep hacking on it.

@syntagmatic
Copy link
Author

Great. This is the list of other operators required?

style, classed, property, on, attr, attrTween, style, styleTween

@mbostock
Copy link
Member

Yep. And probably a bunch of refactoring to avoid code duplication between the map and non-map based methods.

@mbostock
Copy link
Member

Merged into #277.

@mbostock mbostock closed this Aug 28, 2011
murtada58 pushed a commit to murtada58/d3 that referenced this pull request Mar 16, 2022
* Adopt type=module

follow changes in d3-format:
* type=module
* add exports
* remove zip
* license: ISC
* update dependencies

* es6 rather than no-undef

* use the "falls through" comment to make eslint's "no-fallthrough" rule happy

* remove Sublime project

* add eslint.json

* stricter eslint

* cleaner imports

* update dependencies

* Update README

Co-authored-by: Mike Bostock <mbostock@gmail.com>
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.

None yet

3 participants