misoproject / d3.chart

A framework for creating reusable charts with d3.js.
http://misoproject.com/d3-chart
MIT License
728 stars 87 forks source link

pass key function to `draw` call #12

Closed iros closed 10 years ago

iros commented 11 years ago

I realized that even though we have transform, we may still want to allow a user to pass a key function that will be used in the dataBind routine.

Given the chart:

d3.chart("MyChart", {});

For example the following calls:

chart.draw([
  { name : "a", value : 12 },
  { name : "b", value : 155 }
]);
chart.draw([
  { id : "a", value : 12 },
  { id : "b", value : 155 }
]);

It would be unfortunate to define a generic transform function on the chart that would just rename the properties id or name. In practice it would be nicer to just call:

chart.draw([
  { name : "a", value : 12 },
  { name : "b", value : 155 }
], function(d) { return d.name; });
chart.draw([
  { id : "a", value : 12 },
  { id : "b", value : 155 }
], function(d) { return d.id; });
jugglinmike commented 11 years ago

Hello @iros!

When we talked about this on Friday, I suggested implementing key as a property of the Layer (instead of an argument to Chart#draw). Now that I'm trying to make this change, I notice a problem with both approaches: d3.chart never invokes d3.selection#data--the user invokes it in dataBind. This means that, while we can't provide an API for a key function, users already have the ability to specify it in their dataBind method.

That said, the API you've outlined would grant a little more flexibility: users could dynamically change the key function with each call to draw. This particular use case seems strange to me, but I'm not sure if you were intending to support it as well.

iros commented 11 years ago

I see your point. I think because we are unsure of how necessary this is, maybe we leave it open for now?

I wrote one example today on the miso website (forget which of the two) where I had defined a getter/setter for the data attribute, but that seemed hackish to me (and it wasn't even for key purposes, just for data retrieval.)

The use case I'm thinking about has to do with a chart being defined genetically and not by a user for their specific needs. I don't know if that's a common enough need. Barring using requests, maybe we leave it open and see what feedback we get?

jugglinmike commented 11 years ago

I think I understand what you're getting at now. Even though the users of a given chart might want to define a custom key function, they cannot do so because they cannot override dataBind. Is this what you're saying?

iros commented 11 years ago

Yes. Precisely. I had run into another issue on account of the same reason today. I had a circle chart that defined a scale that went from the radius to the width-radius. An extended chart had variable radii and I had to reset the domain of that scale but had no access to the dataBind to do so.

Hmm. Maybe that should have happened in the transform... So maybe less of a problem.

jugglinmike commented 11 years ago

Maybe, but I think this still warrants some further thought. That use case it a lot more compelling than my initial interpretation (the crazy dynamic key function).

As implemented, dataBind has two responsibilities:

  1. Binding the data to existing elements (required, endemic to the Layer)
  2. Setting a key function for your data (optional, context sensitive)

A chart author needs to control the first responsibility (and consumers aren't interested in changing it), but the opposite is true of the second responsibility.

At first glance, it looks like we might be limited by d3's API. d3.selection#data makes these two operations atomic, so as you are well aware, you can't have one without the other.

iros commented 11 years ago

Well our options are:

  1. To add a key method to chart that users can call before draw
  2. To pass a key function as the second argument to draw
  3. To separate draw from the data angle and add a data method that takes both.

My favorite probably is still #2. While we are focusing on drawing, we are passing in data and keeping the key function close seems reasonable.

I can also see #1 working but it's adding another method which would effectively be replicating d3s API. That seems redundant somehow and possibly consuming. I don't like #3 but I added it for completeness.

jugglinmike commented 11 years ago

If I understand correctly, the only way #1 would work is if it modified the data itself. If this is true, I don't see how it would be better than using Layer#transform.

#2 would work, but it would require all chart authors to remember to always pass the second argument to d3.selection#data. We might be well-positioned to avoid this requirement, so I'd like to think about it a little more.

I don't understand #3.

jugglinmike commented 11 years ago

For instance, here is an awful idea that probably won't work:

 // draw
 // Bind the data to the layer, make lifecycle selections, and invoke all
 // relevant handlers.
 Layer.prototype.draw = function(data) {
   var bound, entering, events, selection, handlers, eventName, idx, len;

   bound = this.dataBind.call(this.base, data);
+  // If the layer has a custom `key` function, use it to re-bind the data
+  // to the selection.
+  if (this.key) {
+    bound = bound.data(data, this.key);
+  }

   if (!(bound instanceof d3.selection)) {
       throw new Error('Invalid selection defined by `dataBind` method.');
   }
jugglinmike commented 11 years ago

Here is why the above won't work. The dataBind method may get pretty involved (multiple selections, multiple bindings), so any generic attempts to modify its results will fail for non-trivial layers.

jugglinmike commented 10 years ago

Since Layer#dataBind has to support so many different use cases (especially those requiring nested selections), there doesn't seem to be a way for d3.chart to meaningfully hook into d3.selection.data's key function. The best solution in this case then would be to transform the input data to meet the needs of the chart. As discussed in Pull Request #62: Version 0.2 Roadmap, general data manipulation is outside the purview of this library. Instead, users struggling with this issue might want to use a dedicated library (e.g. Miso Dataset or CrossFilter to accomplish that.

Of of version 0.2, d3.chart honors all transform methods defined in a chart's prototype chain along with a method configured directly on the instance object itself. This makes configuring your instance with a custom transform method easy--your method can transform your input data without interfering with the underlying logic of the chart.