feat(animations): refactor ngbTransition #3793
Conversation
There was a problem hiding this comment.
Hey, @fbasso!
LGTM, just left two comments (see below in code):
- could we use the binding for the initial 'collapse' class
- add a comment why we need
reflow() - (+ a bonus one) the build is failing
Cheers,
Max
src/collapse/collapse.ts
Outdated
| constructor(private _element: ElementRef, config: NgbCollapseConfig) { this.animation = config.animation; } | ||
|
|
||
| ngOnInit() { | ||
| const {classList} = this._element.nativeElement; | ||
| classList.add('collapse'); |
There was a problem hiding this comment.
I think this one we should add via the 'class': 'collapse' binding as before, what's the point doing it imperatively, if we could do this declaratively with Angular...
There was a problem hiding this comment.
I would prefer not merged what is done by binding and what is done by the transition engine.
As far as remember, the collapse class was added back just after starting the transition (direction: show), because collapsed is true. The result is there is collapsingand collapseat the same time in the dom.
I will check, but I thing the tests in this PR prevent this, so it should fail.
| (startFn(element, context) || noopFn)(); | ||
| return of(undefined); | ||
| } | ||
| reflow(element); |
There was a problem hiding this comment.
Could you please explain why in a comment? because we'll never remember this later.
|
Thanks ! |
This is a refactor of ngbTransition to fix an issue around the transition detection.
The main problem comes from the order startFn is run and the transition computed style.
It's easier to explain the issue with the collapse widget:
What Bootstrap do :
collapsingclass,Before this PR :
none, we consider that there is no animation and we run the startFn an endFn synchronously.This is wrong, as it's the startFn purpose is to change collapse to collapsing, we can't compute the
transitionproperty ofcollapsingbefore running it.What this PR do:
At the same time, for the collpase widget:
The other transitions tests need to be refactor as well (not done yet).