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

.changeset/happy-camels-march.md

@@ -0,0 +1,5 @@
+---
+'@primer/view-components': minor
+---
+
+Deprecate `data-show-dialog-id` and `data-close-dialog-id`, use `commandfor` & `command` instead.

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)

app/components/primer/alpha/dialog.rb

@@ -62,7 +62,10 @@ class Dialog < Primer::Component
           system_arguments[:classes]
         )
         system_arguments[:id] = "dialog-show-#{@system_arguments[:id]}"
-        system_arguments[:data] = (system_arguments[:data] || {}).merge({ "show-dialog-id": @system_arguments[:id] })
+        system_arguments[:data] = (system_arguments[:data] || {})
+        system_arguments[:type] = :button
+        system_arguments[:commandfor] = @system_arguments[:id]
+        system_arguments[:command] = "show-modal"
         if icon.present?
           Primer::Beta::IconButton.new(icon: icon, **system_arguments)
         else

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", "type": :button, "commandfor": @id, "command": "close") %>
     </div>
   </div>
   <%= filter %>

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,8 @@ function clickHandler(event: Event) {
   // If the user is clicking a valid dialog trigger
   let dialogId = button?.getAttribute('data-show-dialog-id')
   if (dialogId) {
+    // eslint-disable-next-line no-console
+    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 +39,8 @@ function clickHandler(event: Event) {
 
   dialogId = button.getAttribute('data-close-dialog-id') || button.getAttribute('data-submit-dialog-id')
   if (dialogId) {
+    // eslint-disable-next-line no-console
+    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)

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") %>
       </div>
     <% end %>
   </div>

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)

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,8 @@ function dialogInvokerButtonHandler(event: Event) {
   // If the user is clicking a valid dialog trigger
   let dialogId = button?.getAttribute('data-show-dialog-id')
   if (dialogId) {
+    // eslint-disable-next-line no-console
+    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 +63,8 @@ function dialogInvokerButtonHandler(event: Event) {
 
   dialogId = button.getAttribute('data-close-dialog-id') || button.getAttribute('data-submit-dialog-id')
   if (dialogId) {
+    // eslint-disable-next-line no-console
+    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()

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: { "type": :button, "commandfor": "my-dialog", "command": "show-modal" },
     value: "",
     scheme: :danger
   ) %>

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"

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: { "type": :button, "commandfor": "my-dialog", "command": "show-modal" },
     value: "",
     scheme: :danger
   ) %>
@@ -15,7 +15,7 @@
     Are you sure you want to delete this?
   <% end %>
   <%= render(Primer::Alpha::Dialog::Footer.new()) do %>
-    <%= render(Primer::Beta::Button.new(data: { "close-dialog-id": "my-dialog" })) { "Cancel" } %>
+    <%= render(Primer::Beta::Button.new(data: { "type": :button, "commandfor": "my-dialog", "command": "close" })) { "Cancel" } %>
     <%= render(Primer::Beta::Button.new(scheme: :danger)) { "Delete" } %>
   <% end %>
 <% end %>

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

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][command='show-modal'] svg.octicon.octicon-ellipsis")
       end
 
       def test_raises_on_missing_title

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

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

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)