feat(core): add enter and leave animation instructions #62682
Conversation
843db01 to
46066a1
Compare
46066a1 to
ba72fa8
Compare
ba72fa8 to
594389f
Compare
3c38d4a to
056dcb3
Compare
|
Deployed adev-preview for 4d2fc63 to: https://ng-dev-previews-fw--pr-angular-angular-62682-adev-prev-yvy73xs1.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
packages/core/src/animation.ts
Outdated
| animate(el: Element, removeFn: Function): void { | ||
| if (!this.outElements.has(el)) return; | ||
| const details = this.outElements.get(el)!; | ||
| const timeout = setTimeout(() => removeFn(), 4000); |
There was a problem hiding this comment.
4000 feels like a magic number here. Can we use a named a named constant and/or have a comment on why this value. (Would animations longer than 4s just get removed after ther 4s ?)
There was a problem hiding this comment.
Certainly. I've added a const and associated comment about why 4 seconds.
There was a problem hiding this comment.
Also to quickly answer your question here, if you're using an animation function (rather than classes) for animate.leave, you are required to call the complete function to tell the framework when to remove the element. The feedback on the RFC was to have a timeout for safety, and so I went with the same timeout duration as cross document view transitions. So after a 4 second timeout, the element gets removed. If your animation is done before that, no issues.
There was a problem hiding this comment.
What if we can have an injection token that configures it? And the default would still be 4s.
056dcb3 to
72ef660
Compare
This adds the instructions to support enter and leave animations on nodes.
72ef660 to
4d2fc63
Compare
| @@ -667,6 +679,23 @@ export class ElementRef<T = any> { | |||
| nativeElement: T; | |||
| } | |||
|
|
|||
| // @public | |||
| export class ElementRegistry { | |||
There was a problem hiding this comment.
Do we need this symbol in the public API, was it intentionally exposed?
| @@ -96,6 +97,7 @@ | |||
| "EffectRefImpl", | |||
| "EffectScheduler", | |||
| "ElementRef", | |||
| "ElementRegistry", | |||
There was a problem hiding this comment.
This looks unexpected, should we try making it tree-shakable (maybe via using lightweight injection token in the non-tree-shakable part and providing an actual implementation once the feature is brought it via special instructions)?
| * | ||
| * @codeGenApi | ||
| */ | ||
| export function ɵɵanimateEnter(value: string | Function): typeof ɵɵanimateEnter { |
There was a problem hiding this comment.
This function is pretty long, so I was thinking that it might benefit from either having more docs, explaining the overall logic and/or refactor the logic and extract it into multiple functions (I suspect that some of the functions can also be reused across instructions within this file).
| ): LongestAnimation | undefined { | ||
| if (!(event.target instanceof Element)) return; | ||
| const nativeElement = event.target; | ||
| if (typeof nativeElement.getAnimations === 'function') { |
There was a problem hiding this comment.
We have this check in a few places and I was wondering if we are checking for the getAnimations function presence on a given element or it's a test whether it's generally supported in the current environment? If the latter - we can just do it once somewhere and reuse the result.
| @@ -273,6 +281,7 @@ class DefaultDomRenderer2 implements Renderer2 { | |||
| private readonly ngZone: NgZone, | |||
| private readonly platformIsServer: boolean, | |||
| private readonly tracingService: TracingService<TracingSnapshot> | null, | |||
| private readonly registry: ElementRegistry | null, | |||
There was a problem hiding this comment.
This is likely where the ElementRegistry symbol is "leaking" (becomes non-tree-shakable).
There was a problem hiding this comment.
Yeah. The DOM renderer is the key place this registry is needed to delay removal of elements.
This adds the instructions to support enter and leave animations on nodes.
Does this PR introduce a breaking change?