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 key function to draw call #12

Closed
iros opened this issue May 23, 2013 · 10 comments
Closed

pass key function to draw call #12

iros opened this issue May 23, 2013 · 10 comments

Comments

@iros
Copy link
Member

iros commented May 23, 2013

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
Copy link
Member

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
Copy link
Member Author

iros commented May 26, 2013

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
Copy link
Member

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
Copy link
Member Author

iros commented May 26, 2013

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
Copy link
Member

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
Copy link
Member Author

iros commented May 26, 2013

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

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

No branches or pull requests

2 participants