use command/commandfor over custom data-(show|close|submit)-dialog-id

primer · Feb 13, 2025 · 54b75e9 · 54b75e9
1 parent e3e29b0
commit 54b75e9
Show file tree

Hide file tree

Showing 15 changed files with 40 additions and 19 deletions.
diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts
@@ -247,10 +247,10 @@ export class ActionMenuElement extends HTMLElement {
     if (targetIsItem && eventIsActivation) {
       if (this.#potentiallyDisallowActivation(event)) return
 
-      const dialogInvoker = item.closest('[data-show-dialog-id]')
+      const dialogInvoker = item.closest<HTMLButtonElement>('button[commandfor][command=show-modal]')
 
       if (dialogInvoker) {
-        const dialog = this.ownerDocument.getElementById(dialogInvoker.getAttribute('data-show-dialog-id') || '')
+        const dialog = dialogInvoker.commandForElement
 
         if (dialog && this.contains(dialogInvoker)) {
           this.#handleDialogItemActivated(event, dialog)

diff --git a/app/components/primer/alpha/dialog/header.html.erb b/app/components/primer/alpha/dialog/header.html.erb
@@ -13,7 +13,7 @@
       <% end %>
     </div>
     <div class="Overlay-actionWrap">
-      <%= render Primer::Beta::CloseButton.new(classes: "Overlay-closeButton", "data-close-dialog-id": @id) %>
+      <%= render Primer::Beta::CloseButton.new(classes: "Overlay-closeButton", "commandfor": @id, "command": "close") %>
     </div>
   </div>
   <%= filter %>

diff --git a/app/components/primer/alpha/modal_dialog.ts b/app/components/primer/alpha/modal_dialog.ts
@@ -1,5 +1,6 @@
 import {focusTrap} from '@primer/behaviors'
 import {getFocusableChild} from '@primer/behaviors/utils'
+import 'invokers-polyfill'
 
 function focusIfNeeded(elem: HTMLElement | undefined | null) {
   if (document.activeElement !== elem) {
@@ -18,6 +19,7 @@ function clickHandler(event: Event) {
   // If the user is clicking a valid dialog trigger
   let dialogId = button?.getAttribute('data-show-dialog-id')
   if (dialogId) {
+    console.warn('data-show-dialog-id is deprecated. Please use `commandfor` and `command=show-modal`')
     /* eslint-disable-next-line no-restricted-syntax */
     event.stopPropagation()
     const dialog = document.getElementById(dialogId)
@@ -36,6 +38,7 @@ function clickHandler(event: Event) {
 
   dialogId = button.getAttribute('data-close-dialog-id') || button.getAttribute('data-submit-dialog-id')
   if (dialogId) {
+    console.warn('data-close-dialog-id is deprecated. Please use `commandfor` and `command=show-modal`')
     const dialog = document.getElementById(dialogId)
     if (dialog instanceof ModalDialogElement) {
       const dialogIndex = overlayStack.findIndex(ele => ele.id === dialogId)

diff --git a/app/components/primer/alpha/overlay/header.html.erb b/app/components/primer/alpha/overlay/header.html.erb
@@ -12,7 +12,7 @@
     </div>
     <% if @overlay_id %>
       <div class="Overlay-actionWrap">
-        <%= render Primer::Beta::CloseButton.new(classes: "Overlay-closeButton", "popovertarget": @overlay_id, "popovertargetaction": "hide", "data-close-dialog-id": @overlay_id) %>
+        <%= render Primer::Beta::CloseButton.new(classes: "Overlay-closeButton", "popovertarget": @overlay_id, "popovertargetaction": "hide", "commandfor": @overlay_id, "command": "close") %>
       </div>
     <% end %>
   </div>

diff --git a/app/components/primer/alpha/select_panel_element.ts b/app/components/primer/alpha/select_panel_element.ts
@@ -6,6 +6,7 @@ import type {AnchorAlignment, AnchorSide} from '@primer/behaviors'
 import type {LiveRegionElement} from '@primer/live-region-element'
 import '@primer/live-region-element'
 import '@oddbird/popover-polyfill'
+import 'invokers-polyfill'
 import {observeMutationsUntilConditionMet} from '../utils'
 
 type SelectVariant = 'none' | 'single' | 'multiple' | null
@@ -144,7 +145,7 @@ export class SelectPanelElement extends HTMLElement {
   }
 
   get closeButton(): HTMLButtonElement | null {
-    return this.querySelector('button[data-close-dialog-id]')
+    return this.querySelector('button[commandfor][command="close"]')
   }
 
   get invokerLabel(): HTMLElement | null {
@@ -483,10 +484,10 @@ export class SelectPanelElement extends HTMLElement {
     if (targetIsItem && eventIsActivation) {
       if (this.#potentiallyDisallowActivation(event)) return
 
-      const dialogInvoker = item.closest('[data-show-dialog-id]')
+      const dialogInvoker = item.closest<HTMLButtonElement>('button[commandfor][command="show-modal"]')
 
       if (dialogInvoker) {
-        const dialog = this.ownerDocument.getElementById(dialogInvoker.getAttribute('data-show-dialog-id') || '')
+        const dialog = dialogInvoker.commandForElement
 
         if (dialog && this.contains(dialogInvoker) && this.contains(dialog)) {
           this.#handleDialogItemActivated(event, dialog)

diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts
@@ -1,3 +1,5 @@
+import 'invokers-polyfill'
+
 function dialogInvokerButtonHandler(event: Event) {
   const target = event.target as HTMLElement
   const button = target?.closest('button')
@@ -7,6 +9,7 @@ function dialogInvokerButtonHandler(event: Event) {
   // If the user is clicking a valid dialog trigger
   let dialogId = button?.getAttribute('data-show-dialog-id')
   if (dialogId) {
+    console.warn('data-show-dialog-id is deprecated. Please use `commandfor` and `command=show-modal`')
     const dialog = document.getElementById(dialogId)
     if (dialog instanceof HTMLDialogElement) {
       dialog.showModal()
@@ -59,6 +62,7 @@ function dialogInvokerButtonHandler(event: Event) {
 
   dialogId = button.getAttribute('data-close-dialog-id') || button.getAttribute('data-submit-dialog-id')
   if (dialogId) {
+    console.warn('data-close-dialog-id is deprecated. Please use `commandfor` and `command=close`')
     const dialog = document.getElementById(dialogId)
     if (dialog instanceof HTMLDialogElement && dialog.open) {
       dialog.close()

diff --git a/demo/app/views/action_menu/deferred.html.erb b/demo/app/views/action_menu/deferred.html.erb
@@ -5,7 +5,7 @@
   <% list.with_item(
     label: "Show dialog",
     tag: :button,
-    content_arguments: { "data-show-dialog-id": "my-dialog" },
+    content_arguments: { "commandfor": "my-dialog", "command": "show-modal" },
     value: "",
     scheme: :danger
   ) %>

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -53,9 +53,10 @@
     "@github/relative-time-element": "^4.0.0",
     "@github/remote-input-element": "^0.4.0",
     "@github/tab-container-element": "^3.1.2",
-    "@primer/live-region-element": "^0.7.1",
     "@oddbird/popover-polyfill": "^0.5.2",
-    "@primer/behaviors": "^1.3.4"
+    "@primer/behaviors": "^1.3.4",
+    "@primer/live-region-element": "^0.7.1",
+    "invokers-polyfill": "^0.4.8"
   },
   "peerDependencies": {
     "@primer/primitives": "9.x || 10.x"

diff --git a/previews/primer/alpha/action_menu_preview/opens_dialog.html.erb b/previews/primer/alpha/action_menu_preview/opens_dialog.html.erb
@@ -4,7 +4,7 @@
   <% component.with_item(
     label: "Show dialog",
     tag: :button,
-    content_arguments: { "data-show-dialog-id": "my-dialog" },
+    content_arguments: { "commandfor": "my-dialog", "command": "show-modal" },
     value: "",
     scheme: :danger
   ) %>

diff --git a/test/components/alpha/select_panel_test.rb b/test/components/alpha/select_panel_test.rb
@@ -115,7 +115,7 @@ def test_renders_close_button
         render_preview(:default)
 
         panel_id = page.find_css("select-panel").first.attributes["id"].value
-        assert_selector "select-panel button[data-close-dialog-id='#{panel_id}-dialog']"
+        assert_selector "select-panel button[commandfor='#{panel_id}-dialog'][command=close]"
       end
 
       def test_raises_if_remote_strategy_and_hidden_filter_used_together

diff --git a/test/components/primer/alpha/dialog_test.rb b/test/components/primer/alpha/dialog_test.rb
@@ -24,13 +24,13 @@ def test_renders_show_button
           component.with_show_button { "Show" }
         end
 
-        assert_selector("[data-show-dialog-id]")
+        assert_selector("[commandfor][command='show-modal']")
       end
 
       def test_renders_icon_show_button
         render_preview :playground, params: { icon: :ellipsis }
 
-        assert_selector("button[data-show-dialog-id] svg.octicon.octicon-ellipsis")
+        assert_selector("button[commandfor][comand='show-modal'] svg.octicon.octicon-ellipsis")
       end
 
       def test_raises_on_missing_title

diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb
@@ -84,7 +84,7 @@ def open_panel_via_keyboard
     end
 
     def close_dialog
-      find("[data-close-dialog-id][aria-label=Close]").click
+      find("[commandfor][command=close][aria-label=Close]").click
     end
 
     def focus_on_invoker_button

diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb
@@ -9,12 +9,12 @@ class IntegrationDialogTest < System::TestCase
     def click_on_initial_dialog_close_button
       # this simulates capybara's #trigger method, which isn't supported with selenium drivers
       page.evaluate_script(<<~JS)
-        document.querySelector("button[data-close-dialog-id='dialog-one']").dispatchEvent(new Event('click'))
+        document.querySelector("button[commandfor='dialog-one'][command=close]").dispatchEvent(new Event('click'))
       JS
     end
 
     def click_on_nested_dialog_close_button
-      find("button[data-close-dialog-id='dialog-two']").click
+      find("button[commandfor='dialog-two'][command=close]").click
     end
 
     def click_on_nested_dialog_button

diff --git a/test/system/alpha/select_panel_test.rb b/test/system/alpha/select_panel_test.rb
@@ -38,7 +38,7 @@ def focus_on_invoker_button
     end
 
     def click_on_x_button
-      find("[data-close-dialog-id]").click
+      find("[commandfor][command=close]").click
     end
 
     def click_on_item(idx)