fix(nav): correctly work with conditional nav items (#3894)

Fixes a regression in 8.0.0 after the introduction of animations Fixes #3892
ng-bootstrap · Nov 9, 2020 · 1f466ed · 1f466ed
1 parent 17ffdc1
commit 1f466ed
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 11 deletions.
diff --git a/src/nav/nav-outlet.ts b/src/nav/nav-outlet.ts
@@ -9,8 +9,7 @@ import {
   ViewChildren,
   ViewEncapsulation
 } from '@angular/core';
-
-import {distinctUntilChanged, skip, startWith} from 'rxjs/operators';
+import {distinctUntilChanged, skip, startWith, takeUntil} from 'rxjs/operators';
 
 import {ngbNavFadeInNoReflowTransition, ngbNavFadeInTransition, ngbNavFadeOutTransition} from './nav-transition';
 import {ngbRunTransition, NgbTransitionOptions} from '../util/transition/ngbTransition';
@@ -73,25 +72,23 @@ export class NgbNavOutlet implements AfterViewInit {
 
   ngAfterViewInit() {
     // initial display
-    this._activePane = this._getActivePane();
-    this._activePane ?.elRef.nativeElement.classList.add('show');
-    this._activePane ?.elRef.nativeElement.classList.add('active');
+    this._updateActivePane();
 
     // this will be emitted for all 3 types of nav changes: .select(), [activeId] or (click)
     this.nav.navItemChange$
-      .pipe(startWith(this._activePane ?.item || null), distinctUntilChanged(), skip(1))
+      .pipe(takeUntil(this.nav.destroy$), startWith(this._activePane ?.item || null), distinctUntilChanged(), skip(1))
       .subscribe(nextItem => {
       const options: NgbTransitionOptions<undefined> = {animation: this.nav.animation, runningTransition: 'stop'};
 
+      // next panel we're switching to will only appear in DOM after the change detection is done
+      // and `this._panes` will be updated
+      this._cd.detectChanges();
+
       // fading out
       if (this._activePane) {
         ngbRunTransition(this._activePane.elRef.nativeElement, ngbNavFadeOutTransition, options).subscribe(() => {
           const activeItem = this._activePane ?.item;
 
-          // next panel we're switching to will only appear in DOM after the change detection is done
-          // and `this._panes` will be updated
-          this._cd.detectChanges();
-
           this._activePane = this._getPaneForItem(nextItem);
 
           // fading in
@@ -110,10 +107,18 @@ export class NgbNavOutlet implements AfterViewInit {
             this.nav.hidden.emit(activeItem.id);
           }
         });
+      } else {
+        this._updateActivePane();
       }
       });
   }
 
+  private _updateActivePane() {
+    this._activePane = this._getActivePane();
+    this._activePane ?.elRef.nativeElement.classList.add('show');
+    this._activePane ?.elRef.nativeElement.classList.add('active');
+  }
+
   private _getPaneForItem(item: NgbNavItem | null) {
     return this._panes && this._panes.find(pane => pane.item === item) || null;
   }

diff --git a/src/nav/nav.spec.ts b/src/nav/nav.spec.ts
@@ -325,6 +325,35 @@ describe('nav', () => {
     expect(getContent(fixture)).toBeUndefined();
   });
 
+  it(`should work with conditional nav items`, () => {
+    const fixture = createTestComponent(`
+      <ul ngbNav #n="ngbNav" [(activeId)]="activeId" class="nav-tabs">
+        <li [ngbNavItem]="1" *ngIf="visible">
+            <a ngbNavLink>link 1</a>
+            <ng-template ngbNavContent>content 1</ng-template>
+        </li>
+        <li [ngbNavItem]="2" *ngIf="visible">
+            <a ngbNavLink>link 2</a>
+            <ng-template ngbNavContent>content 2</ng-template>
+        </li>
+      </ul>
+      <div [ngbNavOutlet]="n"></div>
+    `);
+
+    fixture.componentInstance.activeId = 2;
+    fixture.detectChanges();
+
+    expect(fixture.componentInstance.visible).toBe(false);
+    expectLinks(fixture, []);
+    expectContents(fixture, []);
+
+    fixture.componentInstance.visible = true;
+    fixture.detectChanges();
+
+    expectLinks(fixture, [false, true]);
+    expectContents(fixture, ['content 2']);
+  });
+
   it(`should change navs with [activeId] binding`, () => {
     const fixture = createTestComponent(`
       <ul ngbNav #n="ngbNav" [activeId]="activeId" class="nav-tabs">
@@ -1398,6 +1427,7 @@ class TestComponent {
   items = [1, 2];
   orientation = 'horizontal';
   roles: 'tablist' | false = 'tablist';
+  visible = false;
   onActiveIdChange = () => {};
   onNavChange = () => {};
   onItemHidden = () => {};

diff --git a/src/nav/nav.ts b/src/nav/nav.ts
@@ -10,6 +10,7 @@ import {
   forwardRef,
   Inject,
   Input,
+  OnDestroy,
   OnInit,
   Output,
   QueryList,
@@ -19,6 +20,7 @@ import {
 import {DOCUMENT} from '@angular/common';
 
 import {Subject} from 'rxjs';
+import {takeUntil} from 'rxjs/operators';
 
 import {isDefined} from '../util/util';
 import {NgbNavConfig} from './nav-config';
@@ -163,7 +165,8 @@ export class NgbNavItem implements AfterContentChecked, OnInit {
     '(keydown.End)': 'onKeyDown($event)'
   }
 })
-export class NgbNav implements AfterContentInit {
+export class NgbNav implements AfterContentInit,
+    OnDestroy {
   static ngAcceptInputType_orientation: string;
   static ngAcceptInputType_roles: boolean | string;
 
@@ -243,6 +246,7 @@ export class NgbNav implements AfterContentInit {
   @ContentChildren(NgbNavItem) items: QueryList<NgbNavItem>;
   @ContentChildren(forwardRef(() => NgbNavLink), {descendants: true}) links: QueryList<NgbNavLink>;
 
+  destroy$ = new Subject<void>();
   navItemChange$ = new Subject<NgbNavItem | null>();
 
   constructor(
@@ -343,6 +347,8 @@ export class NgbNav implements AfterContentInit {
         this._cd.detectChanges();
       }
     }
+
+    this.items.changes.pipe(takeUntil(this.destroy$)).subscribe(() => this._notifyItemChanged(this.activeId));
   }
 
   ngOnChanges({activeId}: SimpleChanges): void {
@@ -351,6 +357,8 @@ export class NgbNav implements AfterContentInit {
     }
   }
 
+  ngOnDestroy() { this.destroy$.next(); }
+
   private _updateActiveId(nextId: any, emitNavChange = true) {
     if (this.activeId !== nextId) {
       let defaultPrevented = false;