Skip to content

Conversation

FGRibreau
Copy link

@FGRibreau FGRibreau commented Sep 15, 2016

Copy/pasting our discussion with @etimberg 👍

there is no plugin call for after rendering fully finished (ie only after the last frame of animation). afterDraw will do after each frame
I would accept a PR adding it (could call it at the same place the onComplete animation callback is called)

So here it is @etimberg !

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@FGRibreau
Copy link
Author

@zachpanz88 what are you thoughts on this? :)

@simonbrunel
Copy link
Member

I'm not @zachpanz88 :p but want to add my thoughts: afterDraw already provides this functionality:

Chart.plugins.register({
    afterDraw: function(instance, easingDecimal) {
        if (easingDecimal !== 1) {
            // drawing during animation
            return;
        }

        // draw the last animation frame (completedDraw)
    }
});

A potential issue is that for the last animation frame, afterDraw and completedDraw will be called and might do exactly the same thing. My feeling, when it touches public APIs, is that adding duplicated ways to do the same thing could be confusing and for sure will be complicated to remove/change it later.

@FGRibreau
Copy link
Author

@simonbrunel you are perfectly right, so maybe a simple documentation update that explains how to listen when the draw is complete is a better idea don't you think?

@simonbrunel
Copy link
Member

That what I would suggest yes :)

We could replace // Easing is for animation by a more detailed note for the whole group of (before|after)(Datasets)?Draw methods (taking easing as parameter) that explain how to test easing to detect the last frame (i.e. render is complete).

@panzarino
Copy link
Contributor

Looks good to me

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Just a note that we should maybe not add a new plugin API since afterDraw provides the same functionality. Instead, the documentation should be updated (see comments). @etimberg @zachpanz88 what do you think?

@panzarino
Copy link
Contributor

My bad, I thought that you meant implement these changes and change the documentation. If afterDraw provides the exact same functionality there is no reason to add anything new.

@FGRibreau FGRibreau closed this Sep 17, 2016
@FGRibreau FGRibreau deleted the ISSUE-3289 branch September 17, 2016 10:36
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

Successfully merging this pull request may close these issues.

4 participants