fix(carousel): change sildes on prev/next/select API calls with OnPush #2776
Conversation
src/carousel/carousel.ts
Outdated
| @@ -202,6 +199,9 @@ export class NgbCarousel implements AfterContentChecked, | |||
| this._start$.next(); | |||
| this.activeId = selectedSlide.id; | |||
| } | |||
|
|
|||
| // we get here after the interval fires or any external API call like next(), prev() or select() | |||
| this._cd.detectChanges(); | |||
There was a problem hiding this comment.
Shouldn't this be markForCheck? What would happen in a scenario where click on prev / next is calling prev / next on a carousel and is adding a new slide (ex. from ngFor). Would the content of a carousel be checked if we just call detect changes on the carousel itself?
There was a problem hiding this comment.
I get your point, nonetheless, what we want to achieve here is to refresh right away. markForCheck would only refresh it on next CD, isn't it correct ?
There was a problem hiding this comment.
markForCheck would only refresh it on next CD, isn't it correct ?
I would say: in the current CD (the one that would be run at the end of JS loop, before any rendering is done). So I believe that this is the behaviour we actually want.
We should add a test for the above scenario to be sure.
Regression introduced after marking carousel OnPush due to lack of tests Fixes ng-bootstrap#2766
maxokorokov
force-pushed
the
careousel/fix/manual-slide-change
branch
from
8dc1f68
to
14907a9
Compare
|
@pkozlowski-opensource, sure shouldn't run CD synchronously right away, will change to Actually you're right, your example won't work with |
|
LGTM. Feel free to merge when CI gets green. |
Regression introduced after marking carousel OnPush due to lack of tests. Now calling
detectChanges()on interval and all public API actions.Fixes #2766