From 784347f61fa8763dbd31aada90e97e5b25d24485 Mon Sep 17 00:00:00 2001
From: Pete Bacon Darwin <pete@bacondarwin.com>
Date: Tue, 6 Jun 2017 07:36:22 +0100
Subject: [PATCH] fix(aio): ensure all views can indicate the active node
 (#17194)

When more than one node matches a url, the last
node defined in the navigation.json file won. This
meant that, for instance, items in both the
TopBarNarrow and the Footer views would not
indicate that they were active.

Now, each url is associated with a map of current
nodes keyed off their view.

Closes #17022
---
 aio/src/app/app.component.html                |  4 +-
 aio/src/app/app.component.ts                  | 19 ++--
 .../app/layout/nav-item/nav-item.component.ts | 10 +-
 .../app/layout/nav-menu/nav-menu.component.ts |  2 +-
 aio/src/app/navigation/navigation.model.ts    | 11 ++-
 .../app/navigation/navigation.service.spec.ts | 98 +++++++++++--------
 aio/src/app/navigation/navigation.service.ts  | 28 +++---
 7 files changed, 99 insertions(+), 73 deletions(-)

diff --git a/aio/src/app/app.component.html b/aio/src/app/app.component.html
index f22ee9eacd..d18571c831 100644
--- a/aio/src/app/app.component.html
+++ b/aio/src/app/app.component.html
@@ -19,8 +19,8 @@
 <md-sidenav-container class="sidenav-container" [class.starting]="isStarting" 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]="currentNode"></aio-nav-menu>
-    <aio-nav-menu [nodes]="sideNavNodes" [currentNode]="currentNode" ></aio-nav-menu>
+    <aio-nav-menu *ngIf="!isSideBySide" [nodes]="topMenuNarrowNodes" [currentNode]="currentNodes?.TopBarNarrow"></aio-nav-menu>
+    <aio-nav-menu [nodes]="sideNavNodes" [currentNode]="currentNodes?.SideNav" ></aio-nav-menu>
 
     <div class="doc-version" title="Angular docs version {{currentDocVersion?.title}}">
       <select (change)="onDocVersionChange($event.target.selectedIndex)">
diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts
index 546c2b2057..76952d9c73 100644
--- a/aio/src/app/app.component.ts
+++ b/aio/src/app/app.component.ts
@@ -2,7 +2,7 @@ import { Component, ElementRef, HostBinding, HostListener, OnInit,
          QueryList, ViewChild, ViewChildren } from '@angular/core';
 import { MdSidenav } from '@angular/material';
 
-import { CurrentNode, NavigationService, NavigationViews, NavigationNode, VersionInfo } from 'app/navigation/navigation.service';
+import { CurrentNodes, NavigationService, NavigationViews, NavigationNode, VersionInfo } from 'app/navigation/navigation.service';
 import { DocumentService, DocumentContents } from 'app/documents/document.service';
 import { DocViewerComponent } from 'app/layout/doc-viewer/doc-viewer.component';
 import { LocationService } from 'app/shared/location.service';
@@ -25,7 +25,7 @@ export class AppComponent implements OnInit {
 
   currentDocument: DocumentContents;
   currentDocVersion: NavigationNode;
-  currentNode: CurrentNode;
+  currentNodes: CurrentNodes;
   currentPath: string;
   docVersions: NavigationNode[];
   dtOn = false;
@@ -59,7 +59,6 @@ export class AppComponent implements OnInit {
   isSideBySide = false;
   private isFetchingTimeout: any;
   private isSideNavDoc = false;
-  private previousNavView: string;
 
   private sideBySideWidth = 992;
   sideNavNodes: NavigationNode[];
@@ -139,17 +138,17 @@ export class AppComponent implements OnInit {
       }
     });
 
-    this.navigationService.currentNode.subscribe(currentNode => {
-      this.currentNode = currentNode;
+    this.navigationService.currentNodes.subscribe(currentNodes => {
+      this.currentNodes = currentNodes;
 
       // Preserve current sidenav open state by default
       let openSideNav = this.sidenav.opened;
+      const isSideNavDoc = !!currentNodes[sideNavView];
 
-      if (this.previousNavView !== currentNode.view) {
-        this.previousNavView = currentNode.view;
+      if (this.isSideNavDoc !== isSideNavDoc) {
         // View type changed. Is it now a sidenav view (e.g, guide or tutorial)?
         // Open if changed to a sidenav doc; close if changed to a marketing doc.
-        openSideNav = this.isSideNavDoc = currentNode.view === sideNavView;
+        openSideNav = this.isSideNavDoc = isSideNavDoc;
       }
       // May be open or closed when wide; always closed when narrow
       this.sideNavToggle(this.isSideBySide ? openSideNav : false);
@@ -253,9 +252,9 @@ export class AppComponent implements OnInit {
     const sideNavOpen = `sidenav-${this.sidenav.opened ? 'open' : 'closed'}`;
     const pageClass = `page-${this.pageId}`;
     const folderClass = `folder-${this.folderId}`;
-    const viewClass = `view-${this.currentNode && this.currentNode.view}`;
+    const viewClasses = Object.keys(this.currentNodes || {}).map(view => `view-${view}`).join(' ');
 
-    this.hostClasses = `${sideNavOpen} ${pageClass} ${folderClass} ${viewClass}`;
+    this.hostClasses = `${sideNavOpen} ${pageClass} ${folderClass} ${viewClasses}`;
   }
 
   // Dynamically change height of table of contents container
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 da393b0069..3159eef9b7 100644
--- a/aio/src/app/layout/nav-item/nav-item.component.ts
+++ b/aio/src/app/layout/nav-item/nav-item.component.ts
@@ -16,9 +16,13 @@ export class NavItemComponent implements OnChanges {
 
   ngOnChanges(changes: SimpleChanges) {
     if (changes['selectedNodes'] || changes['node']) {
-      const ix = this.selectedNodes.indexOf(this.node);
-      this.isSelected = ix !== -1;
-      if (ix !== 0) { this.isExpanded = this.isSelected; }
+      if (this.selectedNodes) {
+        const ix = this.selectedNodes.indexOf(this.node);
+        this.isSelected = ix !== -1;
+        if (ix !== 0) { this.isExpanded = this.isSelected; }
+      } else {
+        this.isSelected = false;
+      }
     }
     this.setClasses();
   }
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 e065f6548d..fac5e9a0d8 100644
--- a/aio/src/app/layout/nav-menu/nav-menu.component.ts
+++ b/aio/src/app/layout/nav-menu/nav-menu.component.ts
@@ -4,7 +4,7 @@ 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">
   </aio-nav-item>`
 })
 export class NavMenuComponent {
diff --git a/aio/src/app/navigation/navigation.model.ts b/aio/src/app/navigation/navigation.model.ts
index 0873fa3632..15f1c879d7 100644
--- a/aio/src/app/navigation/navigation.model.ts
+++ b/aio/src/app/navigation/navigation.model.ts
@@ -19,7 +19,7 @@ export interface NavigationViews {
 /**
  *  Navigation information about a node at specific URL
  *  url: the current URL
- *  view: 'SideNav' | 'TopBar' | 'Footer'
+ *  view: 'SideNav' | 'TopBar' | 'Footer' | etc
  *  nodes: the current node and its ancestor nodes within that view
  */
 export interface CurrentNode {
@@ -28,6 +28,15 @@ export interface CurrentNode {
   nodes: NavigationNode[];
 }
 
+/**
+ * A map of current nodes by view.
+ * This is needed because some urls map to nodes in more than one view.
+ * If a view does not contain a node that matches the current url then the value will be undefined.
+ */
+export interface CurrentNodes {
+  [view: string]: CurrentNode;
+}
+
 export interface VersionInfo {
   raw: string;
   major: number;
diff --git a/aio/src/app/navigation/navigation.service.spec.ts b/aio/src/app/navigation/navigation.service.spec.ts
index be6bece23d..840ed6b155 100644
--- a/aio/src/app/navigation/navigation.service.spec.ts
+++ b/aio/src/app/navigation/navigation.service.spec.ts
@@ -1,7 +1,7 @@
 import { ReflectiveInjector } from '@angular/core';
 import { Http, ConnectionBackend, RequestOptions, BaseRequestOptions, Response, ResponseOptions } from '@angular/http';
 import { MockBackend } from '@angular/http/testing';
-import { CurrentNode, NavigationService, NavigationViews, NavigationNode, VersionInfo } from 'app/navigation/navigation.service';
+import { CurrentNodes, NavigationService, NavigationViews, NavigationNode, VersionInfo } from 'app/navigation/navigation.service';
 import { LocationService } from 'app/shared/location.service';
 import { MockLocationService } from 'testing/location.service';
 import { Logger } from 'app/shared/logger.service';
@@ -113,7 +113,7 @@ describe('NavigationService', () => {
   });
 
   describe('currentNode', () => {
-    let currentNode: CurrentNode;
+    let currentNodes: CurrentNodes;
     let locationService: MockLocationService;
 
     const topBarNodes: NavigationNode[] = [
@@ -138,80 +138,92 @@ describe('NavigationService', () => {
 
     beforeEach(() => {
       locationService = injector.get(LocationService);
-      navService.currentNode.subscribe(selected => currentNode = selected);
+      navService.currentNodes.subscribe(selected => currentNodes = selected);
       backend.connectionsArray[0].mockRespond(createResponse(navJson));
     });
 
     it('should list the side navigation node that matches the current location, and all its ancestors', () => {
       locationService.go('b');
-      expect(currentNode).toEqual({
-        url: 'b',
-        view: 'SideNav',
-        nodes: [
-          sideNavNodes[0].children[0],
-          sideNavNodes[0]
-        ]
+      expect(currentNodes).toEqual({
+        SideNav: {
+          url: 'b',
+          view: 'SideNav',
+          nodes: [
+            sideNavNodes[0].children[0],
+            sideNavNodes[0]
+          ]
+        }
       });
 
       locationService.go('d');
-      expect(currentNode).toEqual({
-        url: 'd',
-        view: 'SideNav',
-        nodes: [
-          sideNavNodes[0].children[0].children[1],
-          sideNavNodes[0].children[0],
-          sideNavNodes[0]
-        ]
+      expect(currentNodes).toEqual({
+        SideNav: {
+          url: 'd',
+          view: 'SideNav',
+          nodes: [
+            sideNavNodes[0].children[0].children[1],
+            sideNavNodes[0].children[0],
+            sideNavNodes[0]
+          ]
+        }
       });
 
       locationService.go('f');
-      expect(currentNode).toEqual({
-        url: 'f',
-        view: 'SideNav',
-        nodes: [ sideNavNodes[1] ]
+      expect(currentNodes).toEqual({
+        SideNav: {
+          url: 'f',
+          view: 'SideNav',
+          nodes: [ sideNavNodes[1] ]
+        }
       });
     });
 
     it('should be a TopBar selected node if the current location is a top menu node', () => {
       locationService.go('features');
-      expect(currentNode).toEqual({
-        url: 'features',
-        view: 'TopBar',
-        nodes: [ topBarNodes[0] ]
+      expect(currentNodes).toEqual({
+        TopBar: {
+          url: 'features',
+          view: 'TopBar',
+          nodes: [ topBarNodes[0] ]
+        }
       });
     });
 
-    it('should be a plain object if no side navigation node matches the current location', () => {
+    it('should be a plain object if no navigation node matches the current location', () => {
       locationService.go('g?search=moo#anchor-1');
-      expect(currentNode).toEqual({
-        url: 'g',
-        view: '',
-        nodes: []
+      expect(currentNodes).toEqual({
+        '': {
+          url: 'g',
+          view: '',
+          nodes: []
+        }
       });
     });
 
     it('should ignore trailing slashes, hashes, and search params on URLs in the navmap', () => {
-      const cnode: CurrentNode = {
-        url: 'c',
-        view: 'SideNav',
-        nodes: [
-          sideNavNodes[0].children[0].children[0],
-          sideNavNodes[0].children[0],
-          sideNavNodes[0]
-        ]
+      const cnode: CurrentNodes = {
+        SideNav: {
+          url: 'c',
+          view: 'SideNav',
+          nodes: [
+            sideNavNodes[0].children[0].children[0],
+            sideNavNodes[0].children[0],
+            sideNavNodes[0]
+          ]
+        }
       };
 
       locationService.go('c');
-      expect(currentNode).toEqual(cnode, 'location: c');
+      expect(currentNodes).toEqual(cnode, 'location: c');
 
       locationService.go('c#foo');
-      expect(currentNode).toEqual(cnode, 'location: c#foo');
+      expect(currentNodes).toEqual(cnode, 'location: c#foo');
 
       locationService.go('c?foo=1');
-      expect(currentNode).toEqual(cnode, 'location: c?foo=1');
+      expect(currentNodes).toEqual(cnode, 'location: c?foo=1');
 
       locationService.go('c#foo?bar=1&baz=2');
-      expect(currentNode).toEqual(cnode, 'location: c#foo?bar=1&baz=2');
+      expect(currentNodes).toEqual(cnode, 'location: c#foo?bar=1&baz=2');
     });
   });
 
diff --git a/aio/src/app/navigation/navigation.service.ts b/aio/src/app/navigation/navigation.service.ts
index 974e6e834a..39783432bd 100644
--- a/aio/src/app/navigation/navigation.service.ts
+++ b/aio/src/app/navigation/navigation.service.ts
@@ -13,8 +13,8 @@ import { LocationService } from 'app/shared/location.service';
 import { CONTENT_URL_PREFIX } from 'app/documents/document.service';
 
 // Import and re-export the Navigation model types
-import { CurrentNode, NavigationNode, NavigationResponse, NavigationViews, VersionInfo } from './navigation.model';
-export { CurrentNode, NavigationNode, NavigationResponse, NavigationViews, VersionInfo } from './navigation.model';
+import { CurrentNodes, NavigationNode, NavigationResponse, NavigationViews, VersionInfo } from './navigation.model';
+export { CurrentNodes, CurrentNode, NavigationNode, NavigationResponse, NavigationViews, VersionInfo } from './navigation.model';
 
 const navigationPath = CONTENT_URL_PREFIX + 'navigation.json';
 
@@ -35,13 +35,13 @@ export class NavigationService {
    * node (if any) that matches the current URL location
    * including its navigation view and its ancestor nodes in that view
    */
-  currentNode: Observable<CurrentNode>;
+  currentNodes: Observable<CurrentNodes>;
 
   constructor(private http: Http, private location: LocationService, private logger: Logger) {
     const navigationInfo = this.fetchNavigationInfo();
     this.navigationViews = this.getNavigationViews(navigationInfo);
 
-    this.currentNode = this.getCurrentNode(this.navigationViews);
+    this.currentNodes = this.getCurrentNodes(this.navigationViews);
     // The version information is packaged inside the navigation response to save us an extra request.
     this.versionInfo = this.getVersionInfo(navigationInfo);
   }
@@ -88,23 +88,23 @@ export class NavigationService {
   }
 
   /**
-   * Get an observable of the current node (the one that matches the current URL)
+   * Get an observable of the current nodes (the ones that match the current URL)
    * We use `publishReplay(1)` because otherwise subscribers will have to wait until the next
    * URL change before they receive an emission.
    * See above for discussion of using `connect`.
    */
-  private getCurrentNode(navigationViews: Observable<NavigationViews>): Observable<CurrentNode> {
-    const currentNode = combineLatest(
+  private getCurrentNodes(navigationViews: Observable<NavigationViews>): Observable<CurrentNodes> {
+    const currentNodes = combineLatest(
       navigationViews.map(views => this.computeUrlToNavNodesMap(views)),
       this.location.currentPath,
 
       (navMap, url) => {
         const urlKey = url.startsWith('api/') ? 'api' : url;
-        return navMap[urlKey] || { view: '', url: urlKey, nodes: [] };
+        return navMap[urlKey] || { '' : { view: '', url: urlKey, nodes: [] }};
       })
       .publishReplay(1);
-    currentNode.connect();
-    return currentNode;
+    currentNodes.connect();
+    return currentNodes;
   }
 
   /**
@@ -114,7 +114,7 @@ export class NavigationService {
    * @param navigation - A collection of navigation nodes that are to be mapped
    */
   private computeUrlToNavNodesMap(navigation: NavigationViews) {
-    const navMap = new Map<string, CurrentNode>();
+    const navMap = new Map<string, CurrentNodes>();
     Object.keys(navigation)
       .forEach(view => navigation[view]
         .forEach(node => this.walkNodes(view, navMap, node)));
@@ -138,7 +138,7 @@ export class NavigationService {
    * patching them and computing their ancestor nodes
    */
   private walkNodes(
-    view: string, navMap: Map<string, CurrentNode>,
+    view: string, navMap: Map<string, CurrentNodes>,
     node: NavigationNode, ancestors: NavigationNode[] = []) {
       const nodes = [node, ...ancestors];
       const url = node.url;
@@ -147,7 +147,9 @@ export class NavigationService {
       // only map to this node if it has a url
       if (url) {
         // Strip off trailing slashes from nodes in the navMap - they are not relevant to matching
-        navMap[url.replace(/\/$/, '')] = { url, view, nodes };
+        const cleanedUrl = url.replace(/\/$/, '');
+        const navMapItem = navMap[cleanedUrl] = navMap[cleanedUrl] || {};
+        navMapItem[view] = { url, view, nodes };
       }
 
       if (node.children) {