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

A more compact attribute syntax? #55

Closed
lashleigh opened this issue Feb 18, 2011 · 15 comments · May be fixed by Skitionek/d3#4
Closed

A more compact attribute syntax? #55

lashleigh opened this issue Feb 18, 2011 · 15 comments · May be fixed by Skitionek/d3#4
Milestone

Comments

@lashleigh
Copy link

I've been using raphael lately and I really like the ability to make a set of attributes, see the example below. Does this functionality exist in d3? Do you think it ever would?

//d3 way to make a simple circle.
p.append("svg:circle")
.attr("cx", 150)
.attr("cy", 140)
.attr("r", 40)
.attr("fill", "#F00")
.attr("stroke", "black");

// The way I would like to do it.
p.append("svg:circle").attr({cx: 250, cy: 240, r: 40, fill: "#F00", stroke:"black"});

@jasondavies
Copy link
Contributor

I like this idea, I thought about suggesting it too! It does take a bit longer to type .attr(...) all the time so a more compact syntax could be good. Are there any downsides?

@mbostock
Copy link
Member

That sounds reasonable to me, although I'd prefer to use a separate method (say, attrs plural) rather than attempting method overloading through argument type inspection. The only downside I see is object literal creation cost, but that's minimal.

There's also the API parsimony downside, which I think is more significant. Presumably we want to extend this to style as well. So should there be a styles method, too?

Or… not that this is a good idea, an apply method that takes an object with both style, attr, and whatever else we want to apply to the selection?

p.append("svg:circle")
    .apply({attr: {cx: 250, cy: 240, r: 40, fill: "#F00", stroke:"black"}});

@jasondavies
Copy link
Contributor

I think .apply({attr: {...}, style: {...}}) starts to look a bit less readable, but maybe it would be useful in specific cases. I guess something like this is super-simple to write anyway e.g.

function apply(node, nodeFunctions) {
  for (var name in nodeFunctions) {
    var f = node[name], args = nodeFunctions[name];
    for (var argName in args) {
      f.call(node, argName, args[argName]);
    }
  }
}

Assumes that {<function>: {<n>: <v>}} becomes <node>.<function>(<n>, <v>)

I'm not sure if it should be included in D3 because then it would affect parsimony (love that word!) as you say :)

@mbostock
Copy link
Member

That's pretty clever. We could add that as d3_selection.apply(nodeFunctions). Although it wouldn't work for the single-argument methods such as html and text.

@jasondavies
Copy link
Contributor

I guess we could add some typeof testing in there so that {html: "", text: ""} works as expected (and numbers too).

@lashleigh
Copy link
Author

Thanks for considering this, I really appreciate it. Let me know if you need anybody to test out a new format, I'd be happy to help.

@NelsonMinar
Copy link
Contributor

An alternate syntax I used initially by accident:
.attr("cx", 150, "cy", 140, "r", 40, "fill", "#F00", "stroke", "black");
It's a bit magic in positions, but seemed natural enough when I typed it.

@mbostock
Copy link
Member

mbostock commented Mar 6, 2011

I'm learning towards the initial recommendation of a map for attr and style, so that we don't have to deal with type-inference for html and text. If we use a separate name for attrs and styles, then we can avoid the method overloading confusion, potentially (although the names look similar).

Although, there's another question here, should you be able to pass a function to attrs that creates a new object for each element? For example:

.attrs(function() { return {"cx": 150}; })

Or can you assign a function to an attribute of the map?

.attrs({"cx": function() { return 150; }})

Supporting both seems like a pain, but I kind of like the idea of allowing a function that generates a map—that way you can actually set different attributes on different elements in the same selection, which might be useful. It also would allow us to build helper classes that set multiple attributes (although we can do that already using call).

@mbostock
Copy link
Member

See also discussion in #65.

@brettz9
Copy link

brettz9 commented Mar 23, 2011

I think this compact approach would make sense for "style" and "on" as well, and the second argument to append (to eliminate need for attr/s in many cases).

@mbostock
Copy link
Member

So, having a single argument version of attr, style and on sounds reasonable, rather than creating plural-named versions of these methods that look almost identical.

Supporting a single-argument function that generates a map sounds less desirable than a single-argument map; and if we take a single-argument map, then we should allow the values to be functions for consistency with the existing interface. 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).

Lastly, I'm against append taking an additional argument. Then we'd need a third argument to insert, too, and it makes attributes better supported than styles. I'd like them to be equivalent, and I think it's reasonable to require one attr to set attributes. :)

@brettz9
Copy link

brettz9 commented Mar 23, 2011

Great, that sounds cool to me, thanks!

Brett

On 3/23/2011 12:53 PM, mbostock wrote:

So, having a single argument version of attr, style and on sounds reasonable, rather than creating plural-named versions of these methods that look almost identical.

Supporting a single-argument function that generates a map sounds less desirable than a single-argument map; and if we take a single-argument map, then we should allow the values to be functions for consistency with the existing interface. 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).

Lastly, I'm against append taking an additional argument. Then we'd need a third argument to insert, too, and it makes attributes better supported than styles. I'd like them to be equivalent, and I think it's reasonable to require one attr to set attributes. :)

@brettz9
Copy link

brettz9 commented Mar 23, 2011

On 3/23/2011 12:53 PM, mbostock wrote:

Lastly, I'm against append taking an additional argument. Then we'd need a third argument to insert, too, and it makes attributes better supported than styles. I'd like them to be equivalent, and I think it's reasonable to require one attr to set attributes. :)

Sure, it's reasonable, but it's used a lot, and I think attributes tend
to be associated pretty readily with the element named before them (as
in HTML/SVG).

I'd think that styles could also be treated as an attribute if "style"
were specified as a key in the map. Yes, it would mean styles wouldn't
get first class treatment, but it seems to me it should be easy for
people to accept that styles inherently require more complexity (and
thus deeper nesting) compared to other attributes (which "style"
essentially is) since they have their own properties--unless you were to
risk name conflicts by checking for style properties on the map. But, in
any case, styles would still be possible in this scenario, perhaps
meeting your concern?

But whatever you like is ok with me.

Thanks,
Brett

@mbostock
Copy link
Member

mbostock commented Jun 7, 2011

See pull request #179. Should be able to incorporate this into the next release!

@mbostock
Copy link
Member

mbostock commented Aug 5, 2012

Folding this discussion into #277.

@mbostock mbostock closed this as completed Aug 5, 2012
murtada58 pushed a commit to murtada58/d3 that referenced this issue Mar 16, 2022
Fix missing values for defaultOrder and offsetZero
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants