From 2d5623911a6c3478419a2005517d3ae321efa88a Mon Sep 17 00:00:00 2001
From: Ward Bell <wardbell@hotmail.com>
Date: Thu, 8 Jun 2017 15:46:32 -0700
Subject: [PATCH] fix(aio): sidebar folder state after select item
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes #17245 and #17253

When the user selects a doc item in the side nav:
1) expand folder(s) leading to the selected doc item
2) on a wide display, keep other already expanded folders open
3) on narrow (mobile) display, collapse other expanded folders

Used to do (3) when wide. Issue #17245 asks for (2).

That logic was bypassed for selected node when we allowed headers to have content
because that unintentionally expanded the header’s folder when selected.
Because the selected node is no longer a header with content, removing this exclusion
also means that folders are expanded/collapsed with above logic even for API pages.
---
 aio/e2e/app.e2e-spec.ts                       |   4 +-
 aio/src/app/app.component.html                |   4 +-
 .../layout/nav-item/nav-item.component.html   |   2 +-
 .../nav-item/nav-item.component.spec.ts       | 307 ++++++++++--------
 .../app/layout/nav-item/nav-item.component.ts |  13 +-
 .../app/layout/nav-menu/nav-menu.component.ts |   5 +-
 6 files changed, 187 insertions(+), 148 deletions(-)

diff --git a/aio/e2e/app.e2e-spec.ts b/aio/e2e/app.e2e-spec.ts
index bb6df2b2e4..eb3daaa3ab 100644
--- a/aio/e2e/app.e2e-spec.ts
+++ b/aio/e2e/app.e2e-spec.ts
@@ -27,9 +27,7 @@ describe('site App', function() {
     // Show the menu
     page.docsMenuLink.click();
 
-    // Open the tutorial header
-    page.getNavItem(/tutorial/i).click();
-
+    // Tutorial folder should still be expanded because this test runs in wide mode
     // Navigate to the tutorial introduction via a link in the sidenav
     page.getNavItem(/introduction/i).click();
     expect(page.getDocViewerText()).toMatch(/Tutorial: Tour of Heroes/i);
diff --git a/aio/src/app/app.component.html b/aio/src/app/app.component.html
index 9958318de0..52ce34ba0f 100644
--- a/aio/src/app/app.component.html
+++ b/aio/src/app/app.component.html
@@ -18,8 +18,8 @@
 <md-sidenav-container class="sidenav-container" [class.starting]="isStarting" [class.has-floating-toc]="hasFloatingToc" role="main">
 
   <md-sidenav [ngClass]="{'collapsed': !isSideBySide }" #sidenav class="sidenav" [opened]="isOpened" [mode]="mode" (open)="updateHostClasses()" (close)="updateHostClasses()">
-    <aio-nav-menu *ngIf="!isSideBySide" [nodes]="topMenuNarrowNodes" [currentNode]="currentNodes?.TopBarNarrow"></aio-nav-menu>
-    <aio-nav-menu [nodes]="sideNavNodes" [currentNode]="currentNodes?.SideNav" ></aio-nav-menu>
+    <aio-nav-menu *ngIf="!isSideBySide" [nodes]="topMenuNarrowNodes" [currentNode]="currentNodes?.TopBarNarrow" [isWide]="false"></aio-nav-menu>
+    <aio-nav-menu [nodes]="sideNavNodes" [currentNode]="currentNodes?.SideNav" [isWide]="isSideBySide"></aio-nav-menu>
 
     <div class="doc-version" title="Angular docs version {{currentDocVersion?.title}}">
       <aio-select (change)="onDocVersionChange($event.index)" [options]="docVersions" [selected]="docVersions && docVersions[0]"></aio-select>
diff --git a/aio/src/app/layout/nav-item/nav-item.component.html b/aio/src/app/layout/nav-item/nav-item.component.html
index e319889f9b..d54cce4c58 100644
--- a/aio/src/app/layout/nav-item/nav-item.component.html
+++ b/aio/src/app/layout/nav-item/nav-item.component.html
@@ -20,7 +20,7 @@
   </button>
 
   <div class="heading-children" [ngClass]="classes">
-    <aio-nav-item *ngFor="let node of node.children" [level]="level + 1"
+    <aio-nav-item *ngFor="let node of node.children" [level]="level + 1" [isWide]="isWide"
     [node]="node" [selectedNodes]="selectedNodes"></aio-nav-item>
   </div>
 </div>
diff --git a/aio/src/app/layout/nav-item/nav-item.component.spec.ts b/aio/src/app/layout/nav-item/nav-item.component.spec.ts
index 22379c2388..60080721a3 100644
--- a/aio/src/app/layout/nav-item/nav-item.component.spec.ts
+++ b/aio/src/app/layout/nav-item/nav-item.component.spec.ts
@@ -1,159 +1,196 @@
-
-import { SimpleChange, SimpleChanges } from '@angular/core';
+import { TestBed } from '@angular/core/testing';
+import { By } from '@angular/platform-browser';
+import { SimpleChange, SimpleChanges, NO_ERRORS_SCHEMA } from '@angular/core';
 
 import { NavItemComponent } from './nav-item.component';
 import { NavigationNode } from 'app/navigation/navigation.model';
 
-// Testing the component class behaviors, independent of its template
-// No dependencies. Just new it and test :)
-// Let e2e tests verify how it displays.
-describe('NavItemComponent (class-only)', () => {
+describe('NavItemComponent', () => {
 
-  let component: NavItemComponent;
+  // Testing the component class behaviors, independent of its template
+  // No dependencies. Just new it and test :)
+  // Let e2e tests verify how it displays.
+  describe('(class-only)', () => {
 
-  let selectedNodes: NavigationNode[];
-  let setClassesSpy: jasmine.Spy;
+    let component: NavItemComponent;
 
-  function initialize(nd: NavigationNode) {
-    component.node = nd;
-    onChanges(); // Angular calls when initializing the component
-  }
+    let selectedNodes: NavigationNode[];
+    let setClassesSpy: jasmine.Spy;
 
-  // Enough to triggers component's ngOnChange method
-  function onChanges() {
-    component.ngOnChanges({node: <SimpleChange><any> 'anything' });
-  }
+    function initialize(nd: NavigationNode) {
+      component.node = nd;
+      onChanges(); // Angular calls when initializing the component
+    }
 
-  beforeEach(() => {
+    // Enough to triggers component's ngOnChange method
+    function onChanges() {
+      component.ngOnChanges({node: <SimpleChange><any> 'anything' });
+    }
 
-    component = new NavItemComponent();
-    setClassesSpy = spyOn(component, 'setClasses').and.callThrough();
+    beforeEach(() => {
 
-    // Selected nodes is the selected node and its header ancestors
-    selectedNodes = [
-      { title: 'a' },           // selected node: an item or a header
-      { title: 'parent' },      // selected node's header parent
-      { title: 'grandparent' }, // selected node's header grandparent
-    ];
-    component.selectedNodes = selectedNodes;
-  });
+      component = new NavItemComponent();
+      setClassesSpy = spyOn(component, 'setClasses').and.callThrough();
 
-  describe('should have expected classes when initialized', () => {
-    it('with selected node', () => {
-      initialize(selectedNodes[0]);
-      expect(component.classes).toEqual(
-        // selecting the current node has no effect on expanded state,
-        // even if current node is a header.
-        { 'level-1': true, collapsed: true, expanded: false, selected: true}
-      );
+      // Selected nodes is the selected node and its header ancestors
+      selectedNodes = [
+        { title: 'a' },           // selected node: an item or a header
+        { title: 'parent' },      // selected node's header parent
+        { title: 'grandparent' }, // selected node's header grandparent
+      ];
+      component.selectedNodes = selectedNodes;
     });
 
-    it('with selected node ancestor', () => {
-      initialize(selectedNodes[1]);
-      expect(component.classes).toEqual(
-        // ancestor is a header and should be expanded
-        { 'level-1': true, collapsed: false, expanded: true, selected: true}
-      );
+    describe('should have expected classes when initialized', () => {
+      it('with selected node', () => {
+        initialize(selectedNodes[0]);
+        expect(component.classes).toEqual(
+          // selected node should be expanded even if is a header.
+          { 'level-1': true, collapsed: false, expanded: true, selected: true }
+        );
+      });
+
+      it('with selected node ancestor', () => {
+        initialize(selectedNodes[1]);
+        expect(component.classes).toEqual(
+          // ancestor is a header and should be expanded
+          { 'level-1': true, collapsed: false, expanded: true, selected: true }
+        );
+      });
+
+      it('with other than a selected node or ancestor', () => {
+        initialize({ title: 'x' });
+        expect(component.classes).toEqual(
+          { 'level-1': true, collapsed: true, expanded: false, selected: false }
+        );
+      });
     });
 
-    it('with other than a selected node or ancestor', () => {
-      initialize({ title: 'x' });
-      expect(component.classes).toEqual(
-        { 'level-1': true, collapsed: true, expanded: false, selected: false}
-      );
+    describe('when becomes a non-selected node', () => {
+
+      // this node won't be the selected node when ngOnChanges() called
+      beforeEach(() => component.node = { title: 'x' });
+
+      it('should de-select if previously selected', () => {
+        component.isSelected = true;
+        onChanges();
+        expect(component.isSelected).toBe(false, 'becomes de-selected');
+      });
+
+      it('should collapse if previously expanded in narrow mode', () => {
+        component.isWide = false;
+        component.isExpanded = true;
+        onChanges();
+        expect(component.isExpanded).toBe(false, 'becomes collapsed');
+      });
+
+      it('should remain expanded in wide mode', () => {
+        component.isWide = true;
+        component.isExpanded = true;
+        onChanges();
+        expect(component.isExpanded).toBe(true, 'remains expanded');
+      });
+    });
+
+    describe('when becomes a selected node', () => {
+
+      // this node will be the selected node when ngOnChanges() called
+      beforeEach(() => component.node = selectedNodes[0]);
+
+      it('should select when previously not selected', () => {
+        component.isSelected = false;
+        onChanges();
+        expect(component.isSelected).toBe(true, 'becomes selected');
+      });
+
+      it('should expand the current node or keep it expanded', () => {
+        component.isExpanded = false;
+        onChanges();
+        expect(component.isExpanded).toBe(true, 'becomes true');
+
+        component.isExpanded = true;
+        onChanges();
+        expect(component.isExpanded).toBe(true, 'remains true');
+      });
+    });
+
+    describe('when becomes a selected ancestor node', () => {
+
+      // this node will be a selected node ancestor header when ngOnChanges() called
+      beforeEach(() => component.node = selectedNodes[2]);
+
+      it('should select when previously not selected', () => {
+        component.isSelected = false;
+        onChanges();
+        expect(component.isSelected).toBe(true, 'becomes selected');
+      });
+
+      it('should always expand this header', () => {
+        component.isExpanded = false;
+        onChanges();
+        expect(component.isExpanded).toBe(true, 'becomes expanded');
+
+        component.isExpanded = false;
+        onChanges();
+        expect(component.isExpanded).toBe(true, 'stays expanded');
+      });
+    });
+
+    describe('when headerClicked()', () => {
+      // current node doesn't matter in these tests.
+
+      it('should expand when headerClicked() and previously collapsed', () => {
+        component.isExpanded = false;
+        component.headerClicked();
+        expect(component.isExpanded).toBe(true, 'should be expanded');
+      });
+
+      it('should collapse when headerClicked() and previously expanded', () => {
+        component.isExpanded = true;
+        component.headerClicked();
+        expect(component.isExpanded).toBe(false, 'should be collapsed');
+      });
+
+      it('should not change isSelected when headerClicked()', () => {
+        component.isSelected = true;
+        component.headerClicked();
+        expect(component.isSelected).toBe(true, 'remains selected');
+
+        component.isSelected = false;
+        component.headerClicked();
+        expect(component.isSelected).toBe(false, 'remains not selected');
+      });
+
+      it('should set classes', () => {
+        component.headerClicked();
+        expect(setClassesSpy).toHaveBeenCalled();
+      });
     });
   });
 
-  describe('when becomes a non-selected node', () => {
+  describe('(via TestBed)', () => {
+    it('should pass the `isWide` property to all child nav-items', () => {
+      TestBed.configureTestingModule({
+        declarations: [NavItemComponent],
+        schemas: [NO_ERRORS_SCHEMA]
+      });
+      const fixture = TestBed.createComponent(NavItemComponent);
+      fixture.componentInstance.node = {
+        title: 'x',
+        children: [{ title: 'a' }, { title: 'b' }]
+      };
 
-    // this node won't be the selected node when ngOnChanges() called
-    beforeEach(() => component.node = { title: 'x' });
+      fixture.componentInstance.isWide = true;
+      fixture.detectChanges();
+      let children = fixture.debugElement.queryAll(By.directive(NavItemComponent));
+      expect(children.length).toEqual(2);
+      children.forEach(child => expect(child.componentInstance.isWide).toBe(true));
 
-    it('should collapse if previously expanded', () => {
-      component.isExpanded = true;
-      onChanges();
-      expect(component.isExpanded).toBe(false, 'becomes collapsed');
-    });
-
-    it('should de-select if previously selected', () => {
-      component.isSelected = true;
-      onChanges();
-      expect(component.isSelected).toBe(false, 'becomes de-selected');
-    });
-  });
-
-  describe('when becomes a selected node', () => {
-
-    // this node will be the selected node when ngOnChanges() called
-    beforeEach(() => component.node = selectedNodes[0]);
-
-    it('should select when previously not selected', () => {
-      component.isSelected = false;
-      onChanges();
-      expect(component.isSelected).toBe(true, 'becomes selected');
-    });
-
-    it('should leave the expanded/collapsed state untouched', () => {
-      component.isExpanded = false;
-      onChanges();
-      expect(component.isExpanded).toBe(false, 'remains false');
-
-      component.isExpanded = true;
-      onChanges();
-      expect(component.isExpanded).toBe(true, 'remains true');
-    });
-  });
-
-  describe('when becomes a selected ancestor node', () => {
-
-    // this node will be a selected node ancestor header when ngOnChanges() called
-    beforeEach(() => component.node = selectedNodes[2]);
-
-    it('should select when previously not selected', () => {
-      component.isSelected = false;
-      onChanges();
-      expect(component.isSelected).toBe(true, 'becomes selected');
-    });
-
-    it('should always expand this header', () => {
-      component.isExpanded = false;
-      onChanges();
-      expect(component.isExpanded).toBe(true, 'becomes expanded');
-
-      component.isExpanded = false;
-      onChanges();
-      expect(component.isExpanded).toBe(true, 'stays expanded');
-    });
-  });
-
-  describe('when headerClicked()', () => {
-    // current node doesn't matter in these tests.
-
-    it('should expand when headerClicked() and previously collapsed', () => {
-      component.isExpanded = false;
-      component.headerClicked();
-      expect(component.isExpanded).toBe(true, 'should be expanded');
-    });
-
-    it('should collapse when headerClicked() and previously expanded', () => {
-      component.isExpanded = true;
-      component.headerClicked();
-      expect(component.isExpanded).toBe(false, 'should be collapsed');
-    });
-
-    it('should not change isSelected when headerClicked()', () => {
-      component.isSelected = true;
-      component.headerClicked();
-      expect(component.isSelected).toBe(true, 'remains selected');
-
-      component.isSelected = false;
-      component.headerClicked();
-      expect(component.isSelected).toBe(false, 'remains not selected');
-    });
-
-    it('should set classes', () => {
-      component.headerClicked();
-      expect(setClassesSpy).toHaveBeenCalled();
+      fixture.componentInstance.isWide = false;
+      fixture.detectChanges();
+      children = fixture.debugElement.queryAll(By.directive(NavItemComponent));
+      expect(children.length).toEqual(2);
+      children.forEach(child => expect(child.componentInstance.isWide).toBe(false));
     });
   });
 });
diff --git a/aio/src/app/layout/nav-item/nav-item.component.ts b/aio/src/app/layout/nav-item/nav-item.component.ts
index 3159eef9b7..fdd5501e58 100644
--- a/aio/src/app/layout/nav-item/nav-item.component.ts
+++ b/aio/src/app/layout/nav-item/nav-item.component.ts
@@ -6,20 +6,23 @@ import { NavigationNode } from 'app/navigation/navigation.model';
   templateUrl: 'nav-item.component.html',
 })
 export class NavItemComponent implements OnChanges {
-  @Input() selectedNodes: NavigationNode[];
-  @Input() node: NavigationNode;
+  @Input() isWide = false;
   @Input() level = 1;
+  @Input() node: NavigationNode;
+  @Input() selectedNodes: NavigationNode[];
 
   isExpanded = false;
   isSelected = false;
   classes: {[index: string]: boolean };
 
   ngOnChanges(changes: SimpleChanges) {
-    if (changes['selectedNodes'] || changes['node']) {
+    if (changes['selectedNodes'] || changes['node'] || changes['isWide']) {
       if (this.selectedNodes) {
         const ix = this.selectedNodes.indexOf(this.node);
-        this.isSelected = ix !== -1;
-        if (ix !== 0) { this.isExpanded = this.isSelected; }
+        this.isSelected = ix !== -1; // this node is the selected node or its ancestor
+        this.isExpanded = this.isSelected || // expand if selected or ...
+          // preserve expanded state when display is wide; collapse in mobile.
+          (this.isWide && this.isExpanded);
       } else {
         this.isSelected = false;
       }
diff --git a/aio/src/app/layout/nav-menu/nav-menu.component.ts b/aio/src/app/layout/nav-menu/nav-menu.component.ts
index fac5e9a0d8..53194f6c96 100644
--- a/aio/src/app/layout/nav-menu/nav-menu.component.ts
+++ b/aio/src/app/layout/nav-menu/nav-menu.component.ts
@@ -4,11 +4,12 @@ import { CurrentNode, NavigationNode } from 'app/navigation/navigation.service';
 @Component({
   selector: 'aio-nav-menu',
   template: `
-  <aio-nav-item *ngFor="let node of filteredNodes" [node]="node" [selectedNodes]="currentNode?.nodes">
+  <aio-nav-item *ngFor="let node of filteredNodes" [node]="node" [selectedNodes]="currentNode?.nodes" [isWide]="isWide">
   </aio-nav-item>`
 })
 export class NavMenuComponent {
   @Input() currentNode: CurrentNode;
-  @Input() nodes: NavigationNode[] ;
+  @Input() isWide = false;
+  @Input() nodes: NavigationNode[];
   get filteredNodes() { return this.nodes ? this.nodes.filter(n => !n.hidden) : []; }
 }