fix(carousel): do change detection when slides change

Closes #2908 Fixes #2900
ng-bootstrap · Dec 6, 2018 · a944d5d · a944d5d
1 parent 8d5397c
commit a944d5d
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 12 deletions.
diff --git a/src/carousel/carousel.spec.ts b/src/carousel/carousel.spec.ts
@@ -193,8 +193,8 @@ describe('ngb-carousel', () => {
        discardPeriodicTasks();
      }));
 
-  it('should mark component for check for API calls', fakeAsync(() => {
-       const html = `
+  it('should mark component for check for API calls', () => {
+    const html = `
       <ngb-carousel #c [interval]="0">
         <ng-template ngbSlide>foo</ng-template>
         <ng-template ngbSlide>bar</ng-template>
@@ -203,15 +203,36 @@ describe('ngb-carousel', () => {
       <button id="next" (click)="c.next(); addNewSlide = true">Next</button>
     `;
 
-       const fixture = createTestComponent(html);
-       const next = fixture.nativeElement.querySelector('#next');
+    const fixture = createTestComponent(html);
+    const next = fixture.nativeElement.querySelector('#next');
 
-       expectActiveSlides(fixture.nativeElement, [true, false]);
+    expectActiveSlides(fixture.nativeElement, [true, false]);
 
-       next.click();
-       fixture.detectChanges();
-       expectActiveSlides(fixture.nativeElement, [false, true, false]);
-     }));
+    next.click();
+    fixture.detectChanges();
+    expectActiveSlides(fixture.nativeElement, [false, true, false]);
+  });
+
+  it('should mark component for check when slides change', () => {
+    const html = `
+      <ngb-carousel #c [interval]="0">
+        <ng-template ngbSlide *ngFor="let s of slides">
+          <div class="slide">{{ s }}</div>
+        </ng-template>
+      </ngb-carousel>
+    `;
+
+    function getSlidesText(element: HTMLElement): string[] {
+      return Array.from(element.querySelectorAll('.carousel-item .slide')).map((el: HTMLElement) => el.innerHTML);
+    }
+
+    const fixture = createTestComponent(html);
+    expect(getSlidesText(fixture.nativeElement)).toEqual(['a', 'b']);
+
+    fixture.componentInstance.slides = ['c', 'd'];
+    fixture.detectChanges();
+    expect(getSlidesText(fixture.nativeElement)).toEqual(['c', 'd']);
+  });
 
   it('should change slide on indicator click', fakeAsync(() => {
        const html = `
@@ -737,6 +758,7 @@ class TestComponent {
   pauseOnHover = true;
   showNavigationArrows = true;
   showNavigationIndicators = true;
+  slides = ['a', 'b'];
   carouselSlideCallBack = (event: NgbSlideEvent) => {};
 }
 

diff --git a/src/carousel/carousel.ts b/src/carousel/carousel.ts
@@ -21,7 +21,7 @@ import {isPlatformBrowser} from '@angular/common';
 
 import {NgbCarouselConfig} from './carousel-config';
 
-import {Subject, timer} from 'rxjs';
+import {merge, Subject, timer} from 'rxjs';
 import {filter, map, switchMap, takeUntil} from 'rxjs/operators';
 
 let nextId = 0;
@@ -79,6 +79,7 @@ export class NgbCarousel implements AfterContentChecked,
     AfterContentInit, OnChanges, OnDestroy {
   @ContentChildren(NgbSlide) slides: QueryList<NgbSlide>;
 
+  private _destroy$ = new Subject<void>();
   private _start$ = new Subject<void>();
   private _stop$ = new Subject<void>();
 
@@ -146,20 +147,22 @@ export class NgbCarousel implements AfterContentChecked,
         this._start$
             .pipe(
                 map(() => this.interval), filter(interval => interval > 0 && this.slides.length > 0),
-                switchMap(interval => timer(interval).pipe(takeUntil(this._stop$))))
+                switchMap(interval => timer(interval).pipe(takeUntil(merge(this._stop$, this._destroy$)))))
             .subscribe(() => this._ngZone.run(() => this.next()));
 
         this._start$.next();
       });
     }
+
+    this.slides.changes.pipe(takeUntil(this._destroy$)).subscribe(() => this._cd.markForCheck());
   }
 
   ngAfterContentChecked() {
     let activeSlide = this._getSlideById(this.activeId);
     this.activeId = activeSlide ? activeSlide.id : (this.slides.length ? this.slides.first.id : null);
   }
 
-  ngOnDestroy() { this._stop$.next(); }
+  ngOnDestroy() { this._destroy$.next(); }
 
   ngOnChanges(changes) {
     if ('interval' in changes && !changes['interval'].isFirstChange()) {