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

completed hook/event #78

Open
knownasilya opened this issue Feb 3, 2014 · 19 comments
Open

completed hook/event #78

knownasilya opened this issue Feb 3, 2014 · 19 comments

Comments

@knownasilya
Copy link

The idea is to have a completed (or similar) hook or event that runs after all of the elements in a layer have been through the merge process. Basically this will give you a needed context of performing anything that needs to be done once the layer has finished rendering.

My use-case is updating the position of another layer, based on how this layer get's rendered, or even repositioning this layer based on it's size, etc..

At the moment it's possible to trigger an event in the merge hook, but this would be fired for every node, or before the layer has finished rendering. Is this a valid use-case, or am I missing something basic that will allow me to accomplish this?

@samselikoff
Copy link

so strange, just came on here to ask about an isDrawn event that fired after the call to draw was finished and saw your issue at the top of the list. My use case is that my insert method would behave differently if the chart was being drawn for the first time, or if it was being updated.

So I'm also interested in whether such a hook exists, both for layers and for the chart as a whole.

@knownasilya
Copy link
Author

@samselikoff sounds like fate 😉 I'm also in agreement about having one for the chart as a whole.

@knuton
Copy link
Contributor

knuton commented Feb 18, 2014

In principle you should be able to accommodate both use cases by simply overriding draw and doing whatever you need to do after the super-call has completed.

@jugglinmike
Copy link
Member

@knuton That's true in principle (so Sam and Ilya aren't stuck today), but if this is a common enough use case, it may make sense for d3.chart to implement it. This would be monumental: the first "native" Chart event!

@knuton
Copy link
Contributor

knuton commented Feb 23, 2014

I'm in favour of it!

@samselikoff
Copy link

@knuton so how would I call super in the draw method?

@jugglinmike
Copy link
Member

@samselikoff You could do this in a few ways. One would be to gain access to the Chart baseclass via d3.chart(), but that's not documented (and something we're hoping to improve in #76). A slightly more robust (if cumbersome) approach would look something like this:

d3.chart("MyChart", {
  initialize: function() {
    this._superDraw = this.draw;
    this.draw = this.drawAndTell;
  },
  drawAndTell: function() {
    var ret = this._superDraw.apply(this, arguments);
    this.trigger('draw');
    return ret;
  }
});

If possible, please share a demo of what you build with us. Seeing real-world use cases really helps the feature design process.

@knuton
Copy link
Contributor

knuton commented Apr 9, 2014

@jugglinmike, it should be supported via __super__, or am I wrong? (See Chart.extend.)

// ...
  draw: function (data) {
    this.constructor.__super__.draw.call(this, data);
    // do your thing
  }
// ...

@jugglinmike
Copy link
Member

@knuton It would, but that attribute is also undocumented, so I don't endorse its usage.

@knuton
Copy link
Contributor

knuton commented Apr 9, 2014

Would you be open to making it official? It is actually a valuable feature with respect to our project as it allows perfect integration with CoffeeScript's class mechanism. (Sorry for borderline OT, might want to take this to a new issue.)

@jugglinmike
Copy link
Member

Maybe! Inheritance patterns in JavaScript are notoriously thorny, and we mostly passed the buck on this by copping extend from Backbone.js . Endorsing the usage of that property would be a divergence from Backbone, so I'd want to be sure that doing so would't promote any bad practices. If we do decide to endorse it, we would probably re-name (or, if CoffeeScript integration is a priority, alias) to something less obnoxious.

@knuton
Copy link
Contributor

knuton commented Apr 10, 2014

Thanks for the clarification!

@samselikoff
Copy link

@jugglinmike Does what you've said here also apply to transform? I'm subclassing a chart and would like to call the super transform method, then do some more stuff to the data.

In this case, would I need to store the original transform method as a variable? Or can I use __super__? Something else?

@jugglinmike
Copy link
Member

@samselikoff transform is special; d3.chart tries to be intelligent about inherited implementations. From the source:

/**
* Call the `transform` method down the inheritance chain, starting with the
* instance and continuing "upward". The result of each transformation should
* be supplied as input to the next.
*
* @private
*/

But I'm not sure this will suit your use case, since you explicitly want to do work after the parent's transform is called. Can you share more details on the data manipulation you're trying to achieve?

@samselikoff
Copy link

I'm updating the domains of my scales in the transform method, and want to make some adjustments in my subclass (force the min y domain to be 0). Is there a better place for this?

@jugglinmike
Copy link
Member

It sounds like using transform as a hook might work for you as-is, since you don't need to modify the data itself, just the chart's state. Just make sure to return the first argument (so it behaves as a "pass through" operation).

@samselikoff
Copy link

Well the base chart sets the Y scale domain as the extent, so that would override any changes I make in my subclass - correct?

@jugglinmike
Copy link
Member

Ah, I didn't realize that your base chart was setting the same chart attribute.

In this case, it sounds like the subclass isn't just extending the parent chart--it's contradicting it. To more clearly reflect this relationship (and to avoid this problem), I would consider re-defining the chart inheritance hierarchy.

Currently, I imagine it looks something like this:

- A
  -> B

...where A is what we've been calling the "Base chart", and B is what we've called the "subclass". Instead, try:

- A'
  -> A
  -> B

...where A' defines the common behavior (basically everything in A right now, minus the Y-scale-domain setting), and A and B each implement the divergent behavior (in the case of A this is just the Y-scale-domain setting; B doesn't need to change from your current implementation).

@ialarmedalien
Copy link

Having a completed or postDraw hook would be handy for things like chart / container resizing. I've currently got a chart that resizes its container if the contents are outside the viewable area; this requires that all the chart layers be drawn so it can calculate the dimensions of the chart. I've implemented the resizing in the merge hook of the last layer to be drawn, which is a bit of a hack; it'd be much better to hang the resizing on a postDraw hook. Similarly, a preDraw hook would be useful for rendering-related tasks that can/should be done before drawing layers; the transform function can be used/abused for this purpose but I'd prefer to reserve transform for data processing, rather than mixing it with rendering code.

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

No branches or pull requests

5 participants