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

Delay animations until attached #7370

Merged
merged 10 commits into from
May 20, 2020
Merged

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 18, 2020

Played around a bit with animation delaying, for issue #7290

Pen: https://codepen.io/kurkle/pen/MWaowYj

Update: added container attachment detection and also delayed initial update to happen after atteched.

@benmccann
Copy link
Contributor

I didn't look super closely, but the overall idea seems reasonable to me.

canvas needs to be detached, initial parent detachment is not detected

Is that something that would be hard to implement?

canvas is going to be 300x150 and the animation starts from there.

This part seemed unexpected to me. I'd expect that the bars in the codepen should be handled just like the normal initial animation reset case instead of a resize

@kurkle kurkle marked this pull request as ready for review May 18, 2020 21:44
@@ -953,8 +961,10 @@ export default class Chart {
}
};

me.platform.addEventListener(me, 'resize', listener);
me.attached = me.platform.addEventListener(me, 'resize', listener);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a breaking API change that we need to document now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think a new return value is breaking, but it could be documented for sure.
Its not something you'd expect addEventListener to return though, that's why I didn't bother to document it.

Maybe the logic should be moved to core from platform:

if (platform.isAttached(me)) {
  platform.addEventListener(me, 'resize', ...)
  platform.addEventListener(me, 'detach', ...)
} else {
  platrofm.addEventListener(me, 'attach', ...)
}

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be a good strategy. It would allow for alternate implementations without returning from addEventListener

@@ -277,27 +277,28 @@ function unlistenForResize(proxies) {
* @param {function} listener
Copy link
Member

Choose a reason for hiding this comment

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

Docstring needs an update

@etimberg etimberg merged commit cfb5fba into chartjs:master May 20, 2020
@etimberg etimberg added this to the Version 3.0 milestone May 20, 2020
@etimberg etimberg linked an issue May 20, 2020 that may be closed by this pull request
@kurkle kurkle deleted the animate-after-attach branch June 12, 2020 05:38
etimberg pushed a commit that referenced this pull request Sep 1, 2020
* Delay animations until attached
* Detect container detachment
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.

animation does not show
3 participants