From eb97202231e91e0f0bc6bac2f1905381bddc5ac2 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 13 Feb 2025 18:35:07 +0000 Subject: [PATCH 1/2] use command/commandfor over custom data-(show|close|submit)-dialog-id --- .../alpha/action_menu/action_menu_element.ts | 4 ++-- app/components/primer/alpha/dialog.rb | 5 ++++- app/components/primer/alpha/dialog/header.html.erb | 2 +- app/components/primer/alpha/modal_dialog.ts | 5 +++++ .../primer/alpha/overlay/header.html.erb | 2 +- .../primer/alpha/select_panel_element.ts | 7 ++++--- app/components/primer/dialog_helper.ts | 6 ++++++ demo/app/views/action_menu/deferred.html.erb | 2 +- package-lock.json | 14 +++++++++++++- package.json | 5 +++-- .../action_menu_preview/opens_dialog.html.erb | 4 ++-- test/components/alpha/select_panel_test.rb | 2 +- test/components/primer/alpha/dialog_test.rb | 4 ++-- test/system/alpha/action_menu_test.rb | 2 +- test/system/alpha/dialog_test.rb | 4 ++-- test/system/alpha/select_panel_test.rb | 2 +- 16 files changed, 49 insertions(+), 21 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 index 52b1bbc202..7f5519bb6f 100644 --- 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('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.rb b/app/components/primer/alpha/dialog.rb index bc93e76799..186ba7a8e9 100644 --- a/app/components/primer/alpha/dialog.rb +++ b/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 diff --git a/app/components/primer/alpha/dialog/header.html.erb b/app/components/primer/alpha/dialog/header.html.erb index 7ea322f546..65dd9741f9 100644 --- a/app/components/primer/alpha/dialog/header.html.erb +++ b/app/components/primer/alpha/dialog/header.html.erb @@ -13,7 +13,7 @@ <% end %>
- <%= 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") %>
<%= filter %> diff --git a/app/components/primer/alpha/modal_dialog.ts b/app/components/primer/alpha/modal_dialog.ts index d23d61658c..f8f3656ffd 100644 --- 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,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) diff --git a/app/components/primer/alpha/overlay/header.html.erb b/app/components/primer/alpha/overlay/header.html.erb index df67656206..9b0f779577 100644 --- a/app/components/primer/alpha/overlay/header.html.erb +++ b/app/components/primer/alpha/overlay/header.html.erb @@ -12,7 +12,7 @@ <% if @overlay_id %>
- <%= 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") %>
<% end %> diff --git a/app/components/primer/alpha/select_panel_element.ts b/app/components/primer/alpha/select_panel_element.ts index 5b7dd86dd1..2c5692cb24 100644 --- 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('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 index 1382029bd1..c45939f5a1 100644 --- 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,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() diff --git a/demo/app/views/action_menu/deferred.html.erb b/demo/app/views/action_menu/deferred.html.erb index 59e1129010..5957eddcfb 100644 --- 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: { "type": :button, "commandfor": "my-dialog", "command": "show-modal" }, value: "", scheme: :danger ) %> diff --git a/package-lock.json b/package-lock.json index ae85c066c0..87db3f4312 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21,7 +21,8 @@ "@github/tab-container-element": "^3.1.2", "@oddbird/popover-polyfill": "^0.5.2", "@primer/behaviors": "^1.3.4", - "@primer/live-region-element": "^0.7.1" + "@primer/live-region-element": "^0.7.1", + "invokers-polyfill": "^0.4.8" }, "devDependencies": { "@changesets/changelog-github": "^0.5.0", @@ -6563,6 +6564,12 @@ "node": ">= 0.4" } }, + "node_modules/invokers-polyfill": { + "version": "0.4.8", + "resolved": "https://registry.npmjs.org/invokers-polyfill/-/invokers-polyfill-0.4.8.tgz", + "integrity": "sha512-M1injmSmfl2xKjlRaU/Ci6hk+Xma4IzjkTbrho5dacQNSVZMuXj/fNFGORU827+pIzfayPsk1iDAxmWWTzFh1Q==", + "license": "MIT" + }, "node_modules/is-array-buffer": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/is-array-buffer/-/is-array-buffer-3.0.4.tgz", @@ -16297,6 +16304,11 @@ "side-channel": "^1.0.4" } }, + "invokers-polyfill": { + "version": "0.4.8", + "resolved": "https://registry.npmjs.org/invokers-polyfill/-/invokers-polyfill-0.4.8.tgz", + "integrity": "sha512-M1injmSmfl2xKjlRaU/Ci6hk+Xma4IzjkTbrho5dacQNSVZMuXj/fNFGORU827+pIzfayPsk1iDAxmWWTzFh1Q==" + }, "is-array-buffer": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/is-array-buffer/-/is-array-buffer-3.0.4.tgz", diff --git a/package.json b/package.json index 968567cd0e..cb53123e30 100644 --- 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 index d258533d40..2cc738ca04 100644 --- 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: { "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 %> diff --git a/test/components/alpha/select_panel_test.rb b/test/components/alpha/select_panel_test.rb index 36fb472eb2..bfe3a94c08 100644 --- 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 index 6302936c28..168ceb0eca 100644 --- 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][command='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 index 8ec7b8b98d..1ee64b628d 100644 --- 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 index 9a58dd2538..d5576e1d61 100644 --- 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 index f6c3e8f750..3b2151e88b 100644 --- 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) From 53270ba5742a1fc3149f3d5af17d87fe8a861f0b Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 13 Feb 2025 18:46:55 +0000 Subject: [PATCH 2/2] changeset --- .changeset/happy-camels-march.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/happy-camels-march.md diff --git a/.changeset/happy-camels-march.md b/.changeset/happy-camels-march.md new file mode 100644 index 0000000000..f7ad4f1e1a --- /dev/null +++ b/.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.