fix(navigation): sync state with newly added nav items

fixes vmware-clarity#155 vmware-clarity#82 - previously only nav items in the initial render were synced with the top level expanded state - add onslotchange to check for that - move tabindex logic to slot change handler as well - reorder component functions to match best practices - update unit tests to check syncing after component updates
ashleyryan · Aug 23, 2022 · c9536a6 · c9536a6
1 parent ebd5f55
commit c9536a6
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 48 deletions.
diff --git a/projects/core/src/navigation/navigation.element.spec.ts b/projects/core/src/navigation/navigation.element.spec.ts
@@ -125,6 +125,12 @@ describe('cds-navigation', () => {
       itemRefs.forEach(item => {
         expect(item.expanded).toBe(component.expanded);
       });
+
+      component.expanded = true;
+      await componentIsStable(component);
+      itemRefs.forEach(item => {
+        expect(item.expanded).toBe(component.expanded);
+      });
     });
 
     it('to navigationStartRefs', async () => {
@@ -133,12 +139,22 @@ describe('cds-navigation', () => {
       startRefs.forEach(start => {
         expect(start.expandedRoot).toBe(component.expandedRoot);
       });
+
+      component.expanded = true;
+      await componentIsStable(component);
+      startRefs.forEach(start => {
+        expect(start.expandedRoot).toBe(component.expandedRoot);
+      });
     });
 
     it('to rootNavigationStart', async () => {
       await componentIsStable(component);
       const rootStart = component.querySelector<CdsNavigationStart>(':scope > cds-navigation-start');
       expect(rootStart.expanded).toBe(component.expanded);
+
+      component.expanded = true;
+      await componentIsStable(component);
+      expect(rootStart.expanded).toBe(component.expanded);
     });
 
     it('to rootNavigationItems', async () => {
@@ -147,6 +163,28 @@ describe('cds-navigation', () => {
       rootItems.forEach(item => {
         expect(item.expanded).toBe(component.expanded);
       });
+
+      component.expanded = true;
+      await componentIsStable(component);
+      rootItems.forEach(item => {
+        expect(item.expanded).toBe(component.expanded);
+      });
+    });
+
+    it('to new navigation items', async () => {
+      component.expanded = true;
+      await componentIsStable(component);
+
+      const navItem = document.createElement('cds-navigation-item');
+      navItem.id = 'my-new-nav-item';
+      navItem.innerHTML = '<a href="#">My New Nav Item</a>';
+
+      component.appendChild(navItem);
+
+      await componentIsStable(component);
+
+      expect(navItem.expanded).toBe(component.expanded);
+      expect(navItem.tabIndex).toBe(-1);
     });
   });
 });
diff --git a/projects/core/src/navigation/navigation.element.ts b/projects/core/src/navigation/navigation.element.ts
@@ -210,27 +210,72 @@ export class CdsNavigation extends LitElement {
     return Array.from(this.allNavigationElements).filter(n => isVisible(n));
   }
 
-  addStartEventListener() {
-    // This is controlled by the slotchange event on the navigation-start slot
-    // Make sure we reattach the click handler for toggle if consumer changes the start element (e.g *ngIf)
+  render() {
+    return html`<div class="private-host" aria-label="${this.i18n.navigationLabel}" cds-layout="vertical wrap:none">
+      ${this.startTemplate}
+      <slot name="cds-navigation-substart"></slot>
+      <nav class="navigation-body-wrapper">
+        <div .ariaActiveDescendant=${this.ariaActiveDescendant} tabindex="0" id="item-container">
+          <div class="navigation-body" cds-layout="vertical wrap:none align:horizontal-stretch">
+            <slot @slotchange=${this.onSlotChange}></slot>
+          </div>
+        </div>
+      </nav>
+      ${this.endTemplate}
+    </div>`;
+  }
+
+  connectedCallback() {
+    super.connectedCallback();
+    this.role = 'list';
+    this.rootNavigationStart?.addEventListener('focus', this.focusRootStart.bind(this));
+    this.rootNavigationStart?.addEventListener('blur', this.blurRootStart.bind(this));
+    this.rootNavigationStart?.addEventListener('keydown', this.handleRootStartKeys.bind(this));
+  }
+
+  disconnectedCallback() {
+    super.disconnectedCallback();
     if (this.rootNavigationStart) {
-      this.rootNavigationStart.addEventListener('click', this.toggle.bind(this));
+      this.rootNavigationStart.removeEventListener('click', this.toggle.bind(this));
     }
   }
 
   firstUpdated(props: PropertyValues<this>) {
     super.firstUpdated(props);
-    // set all visible navigation elements to tabindex = -1
-    this.allNavigationElements.forEach(item => {
-      setAttributes(item, ['tabindex', '-1']);
-    });
 
     const itemWrapper = this.shadowRoot?.querySelector('#item-container');
     itemWrapper?.addEventListener('focus', this.initItemKeys.bind(this));
     itemWrapper?.addEventListener('keydown', this.handleItemKeys.bind(this));
     itemWrapper?.addEventListener('blur', this.blurItemKeys.bind(this));
   }
 
+  updated(props: PropertyValues<this>) {
+    super.updated(props);
+
+    if (props.has('expanded')) {
+      this.expandedRoot = this.expanded;
+    }
+
+    this.updateChildrenProps();
+  }
+
+  onSlotChange() {
+    this.updateChildrenProps();
+
+    // set all visible navigation elements to tabindex = -1
+    this.allNavigationElements.forEach(item => {
+      setAttributes(item, ['tabindex', '-1']);
+    });
+  }
+
+  addStartEventListener() {
+    // This is controlled by the slotchange event on the navigation-start slot
+    // Make sure we reattach the click handler for toggle if consumer changes the start element (e.g *ngIf)
+    if (this.rootNavigationStart) {
+      this.rootNavigationStart.addEventListener('click', this.toggle.bind(this));
+    }
+  }
+
   private blurItemKeys() {
     if (this.currentActiveItem) {
       removeFocus(this.currentActiveItem);
@@ -360,46 +405,6 @@ export class CdsNavigation extends LitElement {
     });
   }
 
-  connectedCallback() {
-    super.connectedCallback();
-    this.role = 'list';
-    this.rootNavigationStart?.addEventListener('focus', this.focusRootStart.bind(this));
-    this.rootNavigationStart?.addEventListener('blur', this.blurRootStart.bind(this));
-    this.rootNavigationStart?.addEventListener('keydown', this.handleRootStartKeys.bind(this));
-  }
-
-  disconnectedCallback() {
-    super.disconnectedCallback();
-    if (this.rootNavigationStart) {
-      this.rootNavigationStart.removeEventListener('click', this.toggle.bind(this));
-    }
-  }
-
-  render() {
-    return html`<div class="private-host" aria-label="${this.i18n.navigationLabel}" cds-layout="vertical wrap:none">
-      ${this.startTemplate}
-      <slot name="cds-navigation-substart"></slot>
-      <nav class="navigation-body-wrapper">
-        <div .ariaActiveDescendant=${this.ariaActiveDescendant} tabindex="0" id="item-container">
-          <div class="navigation-body" cds-layout="vertical wrap:none align:horizontal-stretch">
-            <slot></slot>
-          </div>
-        </div>
-      </nav>
-      ${this.endTemplate}
-    </div>`;
-  }
-
-  updated(props: PropertyValues<this>) {
-    super.updated(props);
-
-    if (props.has('expanded')) {
-      this.expandedRoot = this.expanded;
-    }
-
-    this.updateChildrenProps();
-  }
-
   updateChildrenProps() {
     if (this.navigationGroupItems) {
       syncPropsForAllItems(Array.from(this.navigationGroupItems), this, { groupItem: true });