Select panel tests (#2954)

Co-authored-by: camertron <[email protected]>

.changeset/beige-knives-complain.md

@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+Small SelectPanel fixes; lots of SelectPanel tests.

app/components/primer/alpha/select_panel.html.erb

@@ -36,7 +36,7 @@
           <% end %>
         <% end %>
       <% end %>
-      <div data-target="select-panel.errorBannerElement" hidden>
+      <div data-target="select-panel.bannerErrorElement" hidden>
         <%= render Primer::Alpha::Banner.new(scheme: :danger, full: true, mt: 2) do %>
           <% if error_content? %>
             <%= error_content %>

app/components/primer/alpha/select_panel.rb

@@ -37,7 +37,7 @@ module Alpha
     # <%= render(
     #   Primer::Alpha::SelectPanel.new(
     #     fetch_strategy: :remote,
-    #     src: search_items_path  # a Rails URL helper
+    #     src: search_items_path  # perhaps a Rails URL helper
     #   )
     # ) %>
     # ```
@@ -122,13 +122,14 @@ module Alpha
     #
     # `SelectPanel`s can be used as form inputs. They behave very similarly to how HTML `<select>` boxes behave, and
     # play nicely with Rails' built-in form mechanisms. Pass arguments via the `form_arguments:` argument, including
-    # the Rails form builder object and the name of the field:
+    # the Rails form builder object and the name of the field. Each list item must also have a value specified in
+    # `content_arguments: { data: { value: } }`.
     #
     # ```erb
     # <% form_with(model: Address.new) do |f| %>
     #   <%= render(Primer::Alpha::SelectPanel.new(form_arguments: { builder: f, name: "country" })) do |menu| %>
     #     <% countries.each do |country|
-    #       <% menu.with_item(label: country.name, data: { value: country.code }) %>
+    #       <% menu.with_item(label: country.name, content_arguments: { data: { value: country.code } }) %>
     #     <% end %>
     #   <% end %>
     # <% end %>
@@ -207,8 +208,6 @@ module Alpha
     #
     # #### State methods
     #
-    # * `showItem(item: Element)`: Shows the item, i.e. makes it visible.
-    # * `hideItem(item: Element)`: Hides the item, i.e. makes it invisible.
     # * `enableItem(item: Element)`: Enables the item, i.e. makes it clickable by the mouse and keyboard.
     # * `disableItem(item: Element)`: Disables the item, i.e. makes it unclickable by the mouse and keyboard.
     # * `checkItem(item: Element)`: Checks the item. Only has an effect in single- and multi-select modes.
@@ -406,7 +405,6 @@ def initialize(
           form_arguments: form_arguments,
           id: "#{@panel_id}-list",
           select_variant: @select_variant,
-          body_id: @body_id,
           role: "listbox",
           aria_selection_variant: @select_variant == :multiple ? :checked : :selected,
           aria: {

app/components/primer/alpha/select_panel_element.ts

@@ -72,7 +72,7 @@ export class SelectPanelElement extends HTMLElement {
   @target ariaLiveContainer: HTMLElement
   @target noResults: HTMLElement
   @target fragmentErrorElement: HTMLElement
-  @target errorBannerElement: HTMLElement
+  @target bannerErrorElement: HTMLElement
   @target bodySpinner: HTMLElement
 
   filterFn?: FilterFn
@@ -194,67 +194,64 @@ export class SelectPanelElement extends HTMLElement {
     this.#softDisableItems()
     updateWhenVisible(this)
 
-    if (this.remoteInput) {
-      this.remoteInput.addEventListener('loadstart', this, {signal})
-      this.remoteInput.addEventListener('loadend', this, {signal})
-    } else {
-      const mutationObserver = new MutationObserver(() => {
-        if (this.remoteInput) {
-          this.remoteInput.addEventListener('loadstart', this, {signal})
-          this.remoteInput.addEventListener('loadend', this, {signal})
-          mutationObserver.disconnect()
-        }
-      })
-
-      mutationObserver.observe(this, {childList: true, subtree: true})
-    }
-
-    if (this.includeFragment) {
-      this.includeFragment.addEventListener('include-fragment-replaced', this, {signal})
-      this.includeFragment.addEventListener('error', this, {signal})
-      this.includeFragment.addEventListener('loadend', this, {signal})
-    } else {
-      const mutationObserver = new MutationObserver(() => {
-        if (this.includeFragment) {
-          this.includeFragment.addEventListener('include-fragment-replaced', this, {signal})
-          this.includeFragment.addEventListener('error', this, {signal})
-          this.includeFragment.addEventListener('loadend', this, {signal})
-          mutationObserver.disconnect()
-        }
-      })
+    this.#waitForCondition(
+      () => Boolean(this.remoteInput),
+      () => {
+        this.remoteInput.addEventListener('loadstart', this, {signal})
+        this.remoteInput.addEventListener('loadend', this, {signal})
+      },
+    )
 
-      mutationObserver.observe(this, {childList: true, subtree: true})
-    }
+    this.#waitForCondition(
+      () => Boolean(this.includeFragment),
+      () => {
+        this.includeFragment.addEventListener('include-fragment-replaced', this, {signal})
+        this.includeFragment.addEventListener('error', this, {signal})
+        this.includeFragment.addEventListener('loadend', this, {signal})
+      },
+    )
 
     this.#dialogIntersectionObserver = new IntersectionObserver(entries => {
       for (const entry of entries) {
         const elem = entry.target
         if (entry.isIntersecting && elem === this.dialog) {
           this.updateAnchorPosition()
-
-          if (this.#fetchStrategy === FetchStrategy.LOCAL) {
-            this.#updateItemVisibility()
-          }
         }
       }
     })
 
-    if (this.dialog) {
-      if (this.getAttribute('data-open-on-load') === 'true') {
-        this.show()
-      }
+    this.#waitForCondition(
+      () => Boolean(this.dialog),
+      () => {
+        if (this.getAttribute('data-open-on-load') === 'true') {
+          this.show()
+        }
+
+        this.#dialogIntersectionObserver.observe(this.dialog)
+        this.dialog.addEventListener('close', this, {signal})
+      },
+    )
+
+    if (this.#fetchStrategy === FetchStrategy.LOCAL) {
+      this.#waitForCondition(
+        () => this.items.length > 0,
+        () => {
+          this.#updateItemVisibility()
+          this.#updateInput()
+        },
+      )
+    }
+  }
 
-      this.#dialogIntersectionObserver.observe(this.dialog)
+  // Waits for condition to return true. If it returns false initially, this function creates a
+  // MutationObserver that calls body() whenever the contents of the component change.
+  #waitForCondition(condition: () => boolean, body: () => void) {
+    if (condition()) {
+      body()
     } else {
       const mutationObserver = new MutationObserver(() => {
-        if (this.dialog) {
-          if (this.getAttribute('data-open-on-load') === 'true') {
-            this.show()
-          }
-
-          this.#dialogIntersectionObserver.observe(this.dialog)
-          mutationObserver.disconnect()
-        }
+        body()
+        mutationObserver.disconnect()
       })
 
       mutationObserver.observe(this, {childList: true, subtree: true})
@@ -355,26 +352,31 @@ export class SelectPanelElement extends HTMLElement {
 
   #checkSelectedItems() {
     for (const item of this.items) {
-      const value = item.getAttribute('data-value')
+      const itemContent = this.#getItemContent(item)
+      if (!itemContent) continue
+
+      const value = itemContent.getAttribute('data-value')
 
       if (value) {
         if (this.#selectedItems.has(value)) {
-          item.setAttribute(this.ariaSelectionType, 'true')
+          itemContent.setAttribute(this.ariaSelectionType, 'true')
         }
       }
     }
     this.#updateInput()
   }
 
   #addSelectedItem(item: SelectPanelItem) {
-    const button = item.querySelector('button')!
-    const value = item.getAttribute('data-value')
+    const itemContent = this.#getItemContent(item)
+    if (!itemContent) return
+
+    const value = itemContent.getAttribute('data-value')
 
     if (value) {
       this.#selectedItems.set(value, {
         value,
-        label: button.querySelector('.ActionListItem-label')?.textContent?.trim(),
-        inputName: button.getAttribute('data-input-name'),
+        label: itemContent.querySelector('.ActionListItem-label')?.textContent?.trim(),
+        inputName: itemContent.getAttribute('data-input-name'),
         element: item,
       })
     }
@@ -437,7 +439,18 @@ export class SelectPanelElement extends HTMLElement {
     }
 
     if (targetIsCloseButton && eventIsActivation) {
-      // #hide will automatically be called by dialog event triggered from `data-close-dialog-id`
+      // hide() will automatically be called by dialog event triggered from `data-close-dialog-id`
+      return
+    }
+
+    if (event.target === this.dialog && event.type === 'close') {
+      this.dispatchEvent(
+        new CustomEvent('panelClosed', {
+          detail: {panel: this},
+          bubbles: true,
+        }),
+      )
+
       return
     }
 
@@ -623,10 +636,10 @@ export class SelectPanelElement extends HTMLElement {
 
       for (const item of this.items) {
         if (filter(item, query)) {
-          this.showItem(item)
+          this.#showItem(item)
           atLeastOneResult = true
         } else {
-          this.hideItem(item)
+          this.#hideItem(item)
         }
       }
     } else {
@@ -637,7 +650,10 @@ export class SelectPanelElement extends HTMLElement {
     this.#maybeAnnounce()
 
     for (const item of this.items) {
-      const value = item.getAttribute('data-value')
+      const itemContent = this.#getItemContent(item)
+      if (!itemContent) continue
+
+      const value = itemContent.getAttribute('data-value')
 
       if (value && !this.#selectedItems.has(value) && this.isItemChecked(item)) {
         this.#addSelectedItem(item)
@@ -666,18 +682,18 @@ export class SelectPanelElement extends HTMLElement {
       return true
     }
 
-    return !this.errorBannerElement.hasAttribute('hidden')
+    return !this.bannerErrorElement.hasAttribute('hidden')
   }
 
   #setErrorState(type: ErrorStateType) {
     let errorElement = this.fragmentErrorElement
 
     if (type === ErrorStateType.BODY) {
       this.fragmentErrorElement?.removeAttribute('hidden')
-      this.errorBannerElement.setAttribute('hidden', '')
+      this.bannerErrorElement.setAttribute('hidden', '')
     } else {
-      errorElement = this.errorBannerElement
-      this.errorBannerElement?.removeAttribute('hidden')
+      errorElement = this.bannerErrorElement
+      this.bannerErrorElement?.removeAttribute('hidden')
       this.fragmentErrorElement?.setAttribute('hidden', '')
     }
 
@@ -690,7 +706,7 @@ export class SelectPanelElement extends HTMLElement {
 
   #clearErrorState() {
     this.fragmentErrorElement?.setAttribute('hidden', '')
-    this.errorBannerElement.setAttribute('hidden', '')
+    this.bannerErrorElement.setAttribute('hidden', '')
   }
 
   #maybeAnnounce() {
@@ -827,6 +843,7 @@ export class SelectPanelElement extends HTMLElement {
     }
 
     this.#updateInput()
+    this.#updateTabIndices()
 
     this.dispatchEvent(
       new CustomEvent('itemActivated', {
@@ -847,12 +864,6 @@ export class SelectPanelElement extends HTMLElement {
 
   hide() {
     this.dialog.close()
-    this.dispatchEvent(
-      new CustomEvent('panelClosed', {
-        detail: {panel: this},
-        bubbles: true,
-      }),
-    )
   }
 
   #setDynamicLabel() {
@@ -979,13 +990,13 @@ export class SelectPanelElement extends HTMLElement {
     }
   }
 
-  hideItem(item: SelectPanelItem | null) {
+  #hideItem(item: SelectPanelItem | null) {
     if (item) {
       item.setAttribute('hidden', 'hidden')
     }
   }
 
-  showItem(item: SelectPanelItem | null) {
+  #showItem(item: SelectPanelItem | null) {
     if (item) {
       item.removeAttribute('hidden')
     }

demo/app/controllers/select_panel_items_controller.rb

@@ -3,9 +3,9 @@
 # :nodoc:
 class SelectPanelItemsController < ApplicationController
   SELECT_PANEL_ITEMS = [
-    { value: 1, selected: true, title: "Phaser", description: "The iconic handheld laser beam" },
-    { value: 2, title: "Photon torpedo", description: "Starship-mounted missile" },
-    { value: 3, title: "Bat'leth", description: "The Klingon warrior's preferred means of achieving honor" },
+    { value: 1, title: "Photon torpedo", description: "Starship-mounted missile" },
+    { value: 2, title: "Bat'leth", description: "The Klingon warrior's preferred means of achieving honor" },
+    { value: 3, selected: true, title: "Phaser", description: "The iconic handheld laser beam" },
     { value: 4, title: "Lightsaber", description: "An elegant weapon for a more civilized age", recent: true },
     { value: 5, title: "Proton pack", description: "Ghostbusting equipment" },
     { value: 6, title: "Sonic screwdriver", description: "The Time Lord's multi-purpose tool" },

previews/primer/alpha/action_menu_preview.rb

@@ -326,7 +326,7 @@ def opens_dialog(menu_id: "menu-1")
       # @label In Scoll container
       #
       def in_scroll_container
-        render_with_template()
+        render_with_template
       end
 
 

previews/primer/alpha/select_panel_preview.rb

@@ -20,6 +20,7 @@ class SelectPanelPreview < ViewComponent::Preview
       # @param anchor_side [Symbol] select [outside_bottom, outside_top, outside_left, outside_right]
       def playground(
         title: "Sci-fi equipment",
+        subtitle: "Various tools from your favorite shows",
         size: :auto,
         simulate_failure: false,
         simulate_no_results: false,
@@ -33,6 +34,7 @@ def playground(
         anchor_side: :outside_bottom
       )
         render_with_template(locals: {
+          subtitle: subtitle,
           system_arguments: {
             title: title,
             size: size,
@@ -216,6 +218,22 @@ def remote_fetch_filter_failure(open_on_load: false)
       def eventually_local_fetch_initial_failure(open_on_load: false)
         render_with_template(locals: { open_on_load: open_on_load })
       end
+
+      # @label Single-select form
+      #
+      # @snapshot interactive
+      # @param open_on_load toggle
+      def single_select_form(open_on_load: false, route_format: :html)
+        render_with_template(locals: { open_on_load: open_on_load, route_format: route_format })
+      end
+
+      # @label Multi-select form
+      #
+      # @snapshot interactive
+      # @param open_on_load toggle
+      def multiselect_form(open_on_load: false, route_format: :html)
+        render_with_template(locals: { open_on_load: open_on_load, route_format: route_format })
+      end
     end
   end
 end