Allow NavList to paginate multiple groups (primer#2406)

Co-authored-by: strackoverflow <strackoverflow@users.noreply.github.com>

.changeset/rotten-trees-occur.md

@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+Fix an issue where multiple groups could not be paginated within the same NavList

app/components/primer/beta/nav_list.ts

@@ -1,44 +1,9 @@
 /* eslint-disable custom-elements/expose-class-on-global */
-import {controller, target, targets} from '@github/catalyst'
+import {controller, targets} from '@github/catalyst'
 
 @controller
 export class NavListElement extends HTMLElement {
   @targets items: HTMLElement[]
-  @target showMoreItem: HTMLElement
-  @targets focusMarkers: HTMLElement[]
-
-  connectedCallback(): void {
-    this.setShowMoreItemState()
-  }
-
-  get showMoreDisabled(): boolean {
-    return this.showMoreItem.hasAttribute('aria-disabled')
-  }
-
-  set showMoreDisabled(value: boolean) {
-    if (value) {
-      this.showMoreItem.setAttribute('aria-disabled', 'true')
-    } else {
-      this.showMoreItem.removeAttribute('aria-disabled')
-    }
-    this.showMoreItem.classList.toggle('disabled', value)
-  }
-
-  set currentPage(value: number) {
-    this.showMoreItem.setAttribute('data-current-page', value.toString())
-  }
-
-  get currentPage(): number {
-    return parseInt(this.showMoreItem.getAttribute('data-current-page') as string) || 1
-  }
-
-  get totalPages(): number {
-    return parseInt(this.showMoreItem.getAttribute('data-total-pages') as string) || 1
-  }
-
-  get paginationSrc(): string {
-    return this.showMoreItem.getAttribute('src') || ''
-  }
 
   selectItemById(itemId: string | null): boolean {
     if (!itemId) return false
@@ -134,55 +99,6 @@ export class NavListElement extends HTMLElement {
     e.stopPropagation()
   }
 
-  private async showMore(e: Event) {
-    e.preventDefault()
-    if (this.showMoreDisabled) return
-    this.showMoreDisabled = true
-    let html
-    try {
-      const paginationURL = new URL(this.paginationSrc, window.location.origin)
-      this.currentPage++
-      paginationURL.searchParams.append('page', this.currentPage.toString())
-      const response = await fetch(paginationURL)
-      if (!response.ok) return
-      html = await response.text()
-      if (this.currentPage === this.totalPages) {
-        this.showMoreItem.hidden = true
-      }
-    } catch (err) {
-      // Ignore network errors
-      this.showMoreDisabled = false
-      this.currentPage--
-      return
-    }
-    const fragment = this.#parseHTML(document, html)
-    fragment?.querySelector('li > a')?.setAttribute('data-targets', 'nav-list.focusMarkers')
-    const listId = (e.target as HTMLElement).closest('button')!.getAttribute('data-list-id')!
-    const list = document.getElementById(listId)!
-    list.append(fragment)
-    this.focusMarkers.pop()?.focus()
-    this.showMoreDisabled = false
-  }
-
-  private setShowMoreItemState() {
-    if (!this.showMoreItem) {
-      return
-    }
-
-    if (this.currentPage < this.totalPages) {
-      this.showMoreItem.hidden = false
-    } else {
-      this.showMoreItem.hidden = true
-    }
-  }
-
-  #parseHTML(document: Document, html: string): DocumentFragment {
-    const template = document.createElement('template')
-    // eslint-disable-next-line github/no-inner-html
-    template.innerHTML = html
-    return document.importNode(template.content, true)
-  }
-
   #findSelectedNavItemById(itemId: string): HTMLElement | null {
     // First we compare the selected link to data-item-id for each nav item
     for (const navItem of this.items) {

app/components/primer/beta/nav_list/group.html.erb

@@ -1,7 +1,9 @@
-<% with_post_list_content do %>
-  <% if show_more_item? %>
-    <%= show_more_item %>
+<nav-list-group>
+  <% with_post_list_content do %>
+    <% if show_more_item? %>
+      <%= show_more_item %>
+    <% end %>
   <% end %>
-<% end %>
 
-<% render_parent %>
+  <% render_parent %>
+</nav-list-group>

app/components/primer/beta/nav_list/group.rb

@@ -25,8 +25,8 @@ class Group < Primer::Alpha::ActionList
           system_arguments[:hidden] = true
           system_arguments[:href] = "#"
           system_arguments[:data] ||= {}
-          system_arguments[:data][:target] = "nav-list.showMoreItem"
-          system_arguments[:data][:action] = "click:nav-list#showMore"
+          system_arguments[:data][:target] = "nav-list-group.showMoreItem"
+          system_arguments[:data][:action] = "click:nav-list-group#showMore"
           system_arguments[:data][:current_page] = "1"
           system_arguments[:data][:total_pages] = pages.to_s
           system_arguments[:label_arguments] = {

app/components/primer/beta/nav_list_group_element.ts

@@ -0,0 +1,97 @@
+import {controller, target, targets} from '@github/catalyst'
+
+@controller
+export class NavListGroupElement extends HTMLElement {
+  @target showMoreItem: HTMLElement
+  @targets focusMarkers: HTMLElement[]
+
+  connectedCallback(): void {
+    this.setShowMoreItemState()
+  }
+
+  get showMoreDisabled(): boolean {
+    return this.showMoreItem.hasAttribute('aria-disabled')
+  }
+
+  set showMoreDisabled(value: boolean) {
+    if (value) {
+      this.showMoreItem.setAttribute('aria-disabled', 'true')
+    } else {
+      this.showMoreItem.removeAttribute('aria-disabled')
+    }
+    this.showMoreItem.classList.toggle('disabled', value)
+  }
+
+  set currentPage(value: number) {
+    this.showMoreItem.setAttribute('data-current-page', value.toString())
+  }
+
+  get currentPage(): number {
+    return parseInt(this.showMoreItem.getAttribute('data-current-page') as string) || 1
+  }
+
+  get totalPages(): number {
+    return parseInt(this.showMoreItem.getAttribute('data-total-pages') as string) || 1
+  }
+
+  get paginationSrc(): string {
+    return this.showMoreItem.getAttribute('src') || ''
+  }
+
+  private async showMore(e: Event) {
+    e.preventDefault()
+    if (this.showMoreDisabled) return
+    this.showMoreDisabled = true
+    let html
+    try {
+      const paginationURL = new URL(this.paginationSrc, window.location.origin)
+      this.currentPage++
+      paginationURL.searchParams.append('page', this.currentPage.toString())
+      const response = await fetch(paginationURL)
+      if (!response.ok) return
+      html = await response.text()
+      if (this.currentPage === this.totalPages) {
+        this.showMoreItem.hidden = true
+      }
+    } catch (err) {
+      // Ignore network errors
+      this.showMoreDisabled = false
+      this.currentPage--
+      return
+    }
+    const fragment = this.#parseHTML(document, html)
+    fragment?.querySelector('li > a')?.setAttribute('data-targets', 'nav-list-group.focusMarkers')
+    const listId = (e.target as HTMLElement).closest('button')!.getAttribute('data-list-id')!
+    const list = document.getElementById(listId)!
+    list.append(fragment)
+    this.focusMarkers.pop()?.focus()
+    this.showMoreDisabled = false
+  }
+
+  private setShowMoreItemState() {
+    if (!this.showMoreItem) {
+      return
+    }
+
+    if (this.currentPage < this.totalPages) {
+      this.showMoreItem.hidden = false
+    } else {
+      this.showMoreItem.hidden = true
+    }
+  }
+
+  #parseHTML(document: Document, html: string): DocumentFragment {
+    const template = document.createElement('template')
+    // eslint-disable-next-line github/no-inner-html
+    template.innerHTML = html
+    return document.importNode(template.content, true)
+  }
+}
+
+declare global {
+  interface Window {
+    NavListGroupElement: typeof NavListGroupElement
+  }
+}
+
+window.NavListGroupElement = NavListGroupElement

app/components/primer/primer.ts

@@ -6,6 +6,7 @@ import './focus_group'
 import './alpha/image_crop'
 import './alpha/modal_dialog'
 import './beta/nav_list'
+import './beta/nav_list_group_element'
 import './alpha/segmented_control'
 import './alpha/toggle_switch'
 import './alpha/tool_tip'

demo/app/controllers/nav_list_items_controller.rb

@@ -5,14 +5,19 @@
 class NavListItemsController < ApplicationController
   layout false
 
-  PAGES = {
-    2 => [
-      { label: "Bachelor Chow", href: "/foods/bachelor-chow" },
-      { label: "LöBrau", href: "/foods/lobrau" }
-    ]
-  }.freeze
+  FOODS = [
+    { label: "Bachelor Chow", href: "/foods/bachelor-chow" },
+    { label: "LöBrau", href: "/foods/lobrau" },
+    { label: "Taco Bellevue Hospital", href: "/foods/taco-bellevue-hospital" },
+    { label: "Olde Fortran", href: "/foods/olde-fortran" },
+    { label: "Space Honey", href: "/foods/space-honey" },
+    { label: "Spice Weasel", href: "/foods/spice-weasel" },
+  ]
 
   def index
-    @data = PAGES[params[:page].to_i]
+    items_per_page = 2
+    # the first page is already shown in the nav list, so we need to offset the starting index
+    start_index = (params[:page].to_i - items_per_page) * items_per_page
+    @data = FOODS.slice(start_index, items_per_page)
   end
 end

previews/primer/beta/nav_list_preview.rb

@@ -106,14 +106,23 @@ def top_level_items
       # @snapshot
       def show_more_item
         render(Primer::Beta::NavList.new(aria: { label: "My favorite foods" })) do |list|
-          list.with_group do |group|
+          list.with_group(id: "foods") do |group|
             group.with_heading(title: "My favorite foods")
             group.with_item(label: "Popplers", href: "/foods/popplers")
             group.with_item(label: "Slurm", href: "/foods/slurm")
             group.with_show_more_item(label: "Show more foods", src: UrlHelpers.nav_list_items_path, pages: 2) do |item|
               item.with_trailing_visual_icon(icon: :plus)
             end
           end
+
+          list.with_group(id: "snacks") do |group|
+            group.with_heading(title: "My favorite snacks")
+            group.with_item(label: "Popplers", href: "/foods/popplers")
+            group.with_item(label: "Slurm", href: "/foods/slurm")
+            group.with_show_more_item(label: "Show more snacks", src: UrlHelpers.nav_list_items_path, pages: 4) do |item|
+              item.with_trailing_visual_icon(icon: :plus)
+            end
+          end
         end
       end
 

test/components/beta/nav_list_test.rb

@@ -179,7 +179,7 @@ def test_max_nesting_depth
       def test_show_more_item
         render_preview(:show_more_item)
 
-        assert_selector("[data-target='nav-list.showMoreItem']", visible: false, text: "Show more")
+        assert_selector("[data-target='nav-list-group.showMoreItem']", visible: false, text: "Show more")
       end
 
       def test_disallow_subitems_and_trailing_action

test/system/beta/nav_list_test.rb

@@ -15,18 +15,30 @@ def test_collapses_group
     def test_shows_more_items
       visit_preview(:show_more_item)
 
-      assert_selector "li.ActionListItem", count: 2
-      assert_selector "li", text: "Popplers"
-      assert_selector "li", text: "Slurm"
+      assert_selector "#foods li.ActionListItem", count: 2
+      assert_selector "#foods li", text: "Popplers"
+      assert_selector "#foods li", text: "Slurm"
 
       # use #find here to wait for the button to become enabled
       find("button", text: "Show more foods").click
 
-      assert_selector "li.ActionListItem", count: 4
-      assert_selector "li", text: "Popplers"
-      assert_selector "li", text: "Slurm"
-      assert_selector "li", text: "Bachelor Chow"
-      assert_selector "li", text: "LöBrau"
+      assert_selector "#foods li.ActionListItem", count: 4
+      assert_selector "#foods li", text: "Popplers"
+      assert_selector "#foods li", text: "Slurm"
+      assert_selector "#foods li", text: "Bachelor Chow"
+      assert_selector "#foods li", text: "LöBrau"
+
+      # ensure second group is unaffected
+      assert_selector "#snacks li.ActionListItem", count: 2
+      assert_selector "#snacks li", text: "Popplers"
+      assert_selector "#snacks li", text: "Slurm"
+
+      find("button", text: "Show more snacks").click
+
+      assert_selector "#snacks li", text: "Popplers"
+      assert_selector "#snacks li", text: "Slurm"
+      assert_selector "#snacks li", text: "Bachelor Chow"
+      assert_selector "#snacks li", text: "LöBrau"
     end
 
     def test_js_api_allows_selecting_item_by_id