From 3c4679cf2c56e72fb54484e20ab1ccc86cfbbe35 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 21 Nov 2023 15:43:50 -0800 Subject: [PATCH 1/6] Don't allow previously hidden items to be checkable; add ActionMenu js API --- .../primer/alpha/action_list/item.rb | 14 ++- .../alpha/action_menu/action_menu_element.ts | 99 +++++++++++++------ .../primer/alpha/action_menu/list.rb | 2 +- 3 files changed, 85 insertions(+), 30 deletions(-) diff --git a/app/components/primer/alpha/action_list/item.rb b/app/components/primer/alpha/action_list/item.rb index 66da459c7e..5e5d08d602 100644 --- a/app/components/primer/alpha/action_list/item.rb +++ b/app/components/primer/alpha/action_list/item.rb @@ -128,7 +128,7 @@ class Item < Primer::Component # @private renders_one :private_content - attr_reader :id, :list, :href, :active, :disabled, :parent + attr_reader :id, :item_id, :list, :href, :active, :disabled, :parent # Whether or not this item is active. # @@ -143,6 +143,7 @@ class Item < Primer::Component # @param list [Primer::Alpha::ActionList] The list that contains this item. Used internally. # @param parent [Primer::Alpha::ActionList::Item] This item's parent item. `nil` if this item is at the root. Used internally. # @param label [String] Item label. If no label is provided, content is used. + # @param item_id [String] An ID that will be attached to the item's `
  • ` element as `data-item-id` for distinguishing between items, perhaps in JavaScript. # @param label_classes [String] CSS classes that will be added to the label. # @param label_arguments [Hash] <%= link_to_system_arguments_docs %> used to construct the label. # @param content_arguments [Hash] <%= link_to_system_arguments_docs %> used to construct the item's anchor or button tag. @@ -161,6 +162,7 @@ class Item < Primer::Component def initialize( list:, label: nil, + item_id: nil, label_classes: nil, label_arguments: {}, content_arguments: {}, @@ -181,6 +183,7 @@ def initialize( @list = list @parent = parent @label = label + @item_id = item_id @href = href || content_arguments[:href] @truncate_label = truncate_label @disabled = disabled @@ -206,6 +209,15 @@ def initialize( @system_arguments[:data] ||= {} @system_arguments[:data][:targets] = "#{list_class.custom_element_name}.items" + @system_arguments[:data] = merge_data( + @system_arguments, { + data: { + targets: "#{list_class.custom_element_name}.items", + **(@item_id ? { item_id: @item_id } : {}) + } + } + ) + @label_arguments = { **label_arguments, classes: class_names( 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 887028331c..b9ad7a5bb4 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -98,7 +98,6 @@ export class ActionMenuElement extends HTMLElement { this.addEventListener('mousedown', this, {signal}) this.#setDynamicLabel() this.#updateInput() - this.#softDisableItems() if (this.includeFragment) { this.includeFragment.addEventListener('include-fragment-replaced', this, { @@ -107,28 +106,6 @@ export class ActionMenuElement extends HTMLElement { } } - #softDisableItems() { - const {signal} = this.#abortController - - for (const item of this.#items) { - item.addEventListener('click', this.#potentiallyDisallowActivation.bind(this), {signal}) - item.addEventListener('keydown', this.#potentiallyDisallowActivation.bind(this), {signal}) - } - } - - #potentiallyDisallowActivation(event: Event) { - if (!this.#isActivation(event)) return - - const item = (event.target as HTMLElement).closest(menuItemSelectors.join(',')) - if (!item) return - - if (item.getAttribute('aria-disabled')) { - event.preventDefault() - event.stopPropagation() - event.stopImmediatePropagation() - } - } - disconnectedCallback() { this.#abortController.abort() } @@ -202,6 +179,13 @@ export class ActionMenuElement extends HTMLElement { const targetIsItem = item !== null if (targetIsItem && eventIsActivation) { + if (item.getAttribute('aria-disabled')) { + event.preventDefault() + event.stopPropagation() + event.stopImmediatePropagation() + return + } + const dialogInvoker = item.closest('[data-show-dialog-id]') if (dialogInvoker) { @@ -214,7 +198,7 @@ export class ActionMenuElement extends HTMLElement { } this.#activateItem(event, item) - this.#handleItemActivated(event, item) + this.#handleItemActivated(item) // Pressing the space key on a button or link will cause the page to scroll unless preventDefault() // is called. While calling preventDefault() appears to have no effect on link navigation, it skips @@ -263,7 +247,7 @@ export class ActionMenuElement extends HTMLElement { dialog.addEventListener('cancel', handleDialogClose, {signal}) } - #handleItemActivated(event: Event, item: Element) { + #handleItemActivated(item: Element) { // Hide popover after current event loop to prevent changes in focus from // altering the target of the event. Not doing this specifically affects // tags. It causes the event to be sent to the currently focused element @@ -327,7 +311,6 @@ export class ActionMenuElement extends HTMLElement { #handleIncludeFragmentReplaced() { if (this.#firstItem) this.#firstItem.focus() - this.#softDisableItems() } // Close when focus leaves menu @@ -410,8 +393,68 @@ export class ActionMenuElement extends HTMLElement { return this.querySelector(menuItemSelectors.join(',')) } - get #items(): HTMLElement[] { - return Array.from(this.querySelectorAll(menuItemSelectors.join(','))) + getItemById(itemId: string): HTMLElement | null { + return this.querySelector(`li[data-item-id="${itemId}"`) + } + + disableItemById(itemId: string) { + const item = this.getItemById(itemId) + + if (item) { + item.classList.add('ActionListItem--disabled') + item.querySelector('.ActionListContent')!.setAttribute('aria-disabled', 'true') + } + } + + enableItemById(itemId: string) { + const item = this.getItemById(itemId) + + if (item) { + item.classList.remove('ActionListItem--disabled') + item.querySelector('.ActionListContent')!.removeAttribute('aria-disabled') + } + } + + hideItemById(itemId: string) { + const item = this.getItemById(itemId) + + if (item) { + item.setAttribute('hidden', 'hidden') + } + } + + showItemById(itemId: string) { + const item = this.getItemById(itemId) + + if (item) { + item.removeAttribute('hidden') + } + } + + checkItemById(itemId: string) { + const item = this.getItemById(itemId) + + if (item && (this.selectVariant === 'single' || this.selectVariant === 'multiple')) { + const itemContent = item.querySelector('.ActionListContent')! + const ariaChecked = itemContent.getAttribute('aria-checked') === 'true' + + if (!ariaChecked) { + this.#handleItemActivated(itemContent) + } + } + } + + uncheckItemById(itemId: string) { + const item = this.getItemById(itemId) + + if (item && (this.selectVariant === 'single' || this.selectVariant === 'multiple')) { + const itemContent = item.querySelector('.ActionListContent')! + const ariaChecked = itemContent.getAttribute('aria-checked') === 'true' + + if (ariaChecked) { + this.#handleItemActivated(itemContent) + } + } } } diff --git a/app/components/primer/alpha/action_menu/list.rb b/app/components/primer/alpha/action_menu/list.rb index d37bef1377..0749a815e6 100644 --- a/app/components/primer/alpha/action_menu/list.rb +++ b/app/components/primer/alpha/action_menu/list.rb @@ -11,7 +11,7 @@ class List < Primer::Component attr_reader :items - # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionMenu::List) %> + # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionList) %> def initialize(**system_arguments) @items = [] @has_group = false From 51ab115d949d0729e5418c7f8485fecf7f63b84b Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 22 Nov 2023 11:53:14 -0800 Subject: [PATCH 2/6] Add JS API --- app/components/primer/alpha/action_menu.rb | 22 ++++ .../alpha/action_menu/action_menu_element.ts | 69 ++++++++++-- previews/primer/alpha/action_menu_preview.rb | 19 ++-- test/system/alpha/action_menu_test.rb | 102 ++++++++++++++++++ 4 files changed, 199 insertions(+), 13 deletions(-) diff --git a/app/components/primer/alpha/action_menu.rb b/app/components/primer/alpha/action_menu.rb index 6704aef5a6..fe9fdd670d 100644 --- a/app/components/primer/alpha/action_menu.rb +++ b/app/components/primer/alpha/action_menu.rb @@ -128,6 +128,28 @@ module Alpha # # Additional information around the keyboard functionality and implementation can be found on the # [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.2/#menu). + # + # ### JavaScript API + # + # `ActionList`s render an `` custom element that exposes behavior to the client. For all these methods, + # `itemId` refers to the value of the `item_id:` argument (see below) that is used to populate the `data-item-id` HTML + # attribute. + # + # #### Query methods + # + # * `isItemChecked(itemId: string): boolean`: Returns `true` if the item is checked, `false` otherwise. + # * `isItemHidden(itemId: string): boolean`: Returns `true` if the item is hidden, `false` otherwise. + # * `isItemDisabled(itemId: string): boolean`: Returns `true` if the item is disabled, `false` otherwise. + # * `getItemById(itemId: string): HTMLElement`: Returns the item's HTML `
  • ` element. + # + # #### State methods + # + # * `showItemById(itemId: string)`: Shows the item, i.e. makes it visible. + # * `hideItemById(itemId: string)`: Hides the item, i.e. makes it invisible. + # * `enableItemById(itemId: string)`: Enables the item, i.e. makes it clickable by the mouse and keyboard. + # * `disableItemById(itemId: string)`: Disables the item, i.e. makes it unclickable by the mouse and keyboard. + # * `checkItemById(itemId: string)`: Checks the item. Only has an effect in single- and multi-select modes. + # * `uncheckItemById(itemId: string)`: Unchecks the item. Only has an effect in multi-select mode, since items cannot be unchecked in single-select mode. class ActionMenu < Primer::Component status :alpha 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 b9ad7a5bb4..337ca72d9d 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -98,6 +98,7 @@ export class ActionMenuElement extends HTMLElement { this.addEventListener('mousedown', this, {signal}) this.#setDynamicLabel() this.#updateInput() + this.#softDisableItems() if (this.includeFragment) { this.includeFragment.addEventListener('include-fragment-replaced', this, { @@ -110,6 +111,32 @@ export class ActionMenuElement extends HTMLElement { this.#abortController.abort() } + #softDisableItems() { + const {signal} = this.#abortController + + for (const item of this.querySelectorAll(validSelectors.join(','))) { + item.addEventListener('click', this.#potentiallyDisallowActivation.bind(this), {signal}) + item.addEventListener('keydown', this.#potentiallyDisallowActivation.bind(this), {signal}) + } + } + + // returns true if activation was prevented + #potentiallyDisallowActivation(event: Event): boolean { + if (!this.#isActivation(event)) return false + + const item = (event.target as HTMLElement).closest(menuItemSelectors.join(',')) + if (!item) return false + + if (item.getAttribute('aria-disabled')) { + event.preventDefault() + event.stopPropagation() + event.stopImmediatePropagation() + return true + } + + return false + } + #isKeyboardActivation(event: Event): boolean { return this.#isKeyboardActivationViaEnter(event) || this.#isKeyboardActivationViaSpace(event) } @@ -179,12 +206,7 @@ export class ActionMenuElement extends HTMLElement { const targetIsItem = item !== null if (targetIsItem && eventIsActivation) { - if (item.getAttribute('aria-disabled')) { - event.preventDefault() - event.stopPropagation() - event.stopImmediatePropagation() - return - } + if (this.#potentiallyDisallowActivation(event)) return const dialogInvoker = item.closest('[data-show-dialog-id]') @@ -311,6 +333,7 @@ export class ActionMenuElement extends HTMLElement { #handleIncludeFragmentReplaced() { if (this.#firstItem) this.#firstItem.focus() + this.#softDisableItems() } // Close when focus leaves menu @@ -393,10 +416,24 @@ export class ActionMenuElement extends HTMLElement { return this.querySelector(menuItemSelectors.join(',')) } + get #items(): HTMLElement[] { + return Array.from(this.querySelectorAll(menuItemSelectors.join(','))) + } + getItemById(itemId: string): HTMLElement | null { return this.querySelector(`li[data-item-id="${itemId}"`) } + isItemDisabled(itemId: string): boolean { + const item = this.getItemById(itemId) + + if (item) { + return item.classList.contains('ActionListItem--disabled') + } else { + return false + } + } + disableItemById(itemId: string) { const item = this.getItemById(itemId) @@ -415,6 +452,16 @@ export class ActionMenuElement extends HTMLElement { } } + isItemHidden(itemId: string): boolean { + const item = this.getItemById(itemId) + + if (item) { + return item.hasAttribute('hidden') + } else { + return false + } + } + hideItemById(itemId: string) { const item = this.getItemById(itemId) @@ -431,6 +478,16 @@ export class ActionMenuElement extends HTMLElement { } } + isItemChecked(itemId: string) { + const item = this.getItemById(itemId) + + if (item) { + return item.querySelector('.ActionListContent')!.getAttribute('aria-checked') === 'true' + } else { + return false + } + } + checkItemById(itemId: string) { const item = this.getItemById(itemId) diff --git a/previews/primer/alpha/action_menu_preview.rb b/previews/primer/alpha/action_menu_preview.rb index 025cd656bd..84a0abbe89 100644 --- a/previews/primer/alpha/action_menu_preview.rb +++ b/previews/primer/alpha/action_menu_preview.rb @@ -148,10 +148,12 @@ def with_icon_button def single_select render(Primer::Alpha::ActionMenu.new(select_variant: :single)) do |menu| menu.with_show_button { "Menu" } - menu.with_item(label: "Fast forward") - menu.with_item(label: "Recursive") - menu.with_item(label: "Ours") - menu.with_item(label: "Resolve") + menu.with_item(label: "Fast forward", item_id: :fast_forward) + menu.with_item(label: "Recursive", item_id: :recursive) + menu.with_item(label: "Ours", item_id: :ours) + menu.with_item(label: "Resolve", item_id: :resolve) + menu.with_item(label: "Disabled", disabled: true, item_id: :disabled) + menu.with_item(label: "Hidden", hidden: true, disabled: true, item_id: :hidden) end end @@ -166,7 +168,8 @@ def multiple_select username: "langermank", full_name: "Katie Langerman", full_name_scheme: :inline, - avatar_arguments: { shape: :square } + avatar_arguments: { shape: :square }, + item_id: :katie ) menu.with_avatar_item( @@ -174,7 +177,8 @@ def multiple_select username: "jonrohan", full_name: "Jon Rohan", full_name_scheme: :inline, - avatar_arguments: { shape: :square } + avatar_arguments: { shape: :square }, + item_id: :jon ) menu.with_avatar_item( @@ -182,7 +186,8 @@ def multiple_select username: "broccolinisoup", full_name: "Armağan Ersöz", full_name_scheme: :inline, - avatar_arguments: { shape: :square } + avatar_arguments: { shape: :square }, + item_id: :armagan ) end end diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 8c0d32efa0..e95e70c9f3 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -17,6 +17,10 @@ def click_on_item(idx) items[idx - 1].click end + def click_on_item_by_id(id) + find("li[data-item-id='#{id}']").click + end + def click_on_first_item click_on_item(1) end @@ -599,5 +603,103 @@ def test_closes_menu_when_open_on_invoker_click click_on_invoker_button refute_selector "action-menu ul li" end + + def test_unhidden_disabled_item_cannot_be_checked + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=hidden]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').showItemById('hidden'); + JS + + assert_selector "li[data-item-id=hidden]" + + click_on_item_by_id("hidden") + refute_selector "li [aria-checked=true]" + end + + def test_hide_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + assert_selector "li[data-item-id=recursive]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').hideItemById('recursive'); + JS + + refute_selector "li[data-item-id=recursive]" + end + + def test_show_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=hidden]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').showItemById('hidden'); + JS + + assert_selector "li[data-item-id=hidden]" + end + + def test_disable_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=resolve] [aria-disabled=true]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').disableItemById('resolve'); + JS + + assert_selector "li[data-item-id=resolve] [aria-disabled=true]" + end + + def test_enable_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + assert_selector "li[data-item-id=disabled] [aria-disabled=true]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').enableItemById('disabled'); + JS + + refute_selector "li[data-item-id=disabled] [aria-disabled=true]" + end + + def test_check_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=recursive] [aria-checked=true]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').checkItemById('recursive'); + JS + + click_on_invoker_button + assert_selector "li[data-item-id=recursive] [aria-checked=true]" + end + + def test_uncheck_item_via_js_api + # use multiple_select preview here because single-select menus do not allow unchecking checked items + visit_preview(:multiple_select) + + click_on_invoker_button + click_on_item_by_id("jon") + + assert_selector "li[data-item-id=jon] [aria-checked=true]" + + page.evaluate_script(<<~JS) + document.querySelector('action-menu').uncheckItemById('jon'); + JS + + refute_selector "li[data-item-id=jon] [aria-checked=true]" + end end end From bfc373495d4042c8dd30253a9c714b8f652b5fb3 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 22 Nov 2023 11:53:53 -0800 Subject: [PATCH 3/6] Add changeset --- .changeset/strange-rings-confess.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/strange-rings-confess.md diff --git a/.changeset/strange-rings-confess.md b/.changeset/strange-rings-confess.md new file mode 100644 index 0000000000..c834c055c1 --- /dev/null +++ b/.changeset/strange-rings-confess.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': minor +--- + +[ActionMenu] Don't allow previously hidden items to be checkable; add JavaScript API From ad6953e0a752ce0c1fd458af8a440f96225c08e2 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 22 Nov 2023 14:41:30 -0800 Subject: [PATCH 4/6] Fire itemActivated event --- app/components/primer/alpha/action_menu.rb | 31 +++++++--- .../alpha/action_menu/action_menu_element.ts | 54 ++++++++-------- test/system/alpha/action_menu_test.rb | 61 ++++++++++++++----- 3 files changed, 94 insertions(+), 52 deletions(-) diff --git a/app/components/primer/alpha/action_menu.rb b/app/components/primer/alpha/action_menu.rb index fe9fdd670d..8b01d622d5 100644 --- a/app/components/primer/alpha/action_menu.rb +++ b/app/components/primer/alpha/action_menu.rb @@ -137,19 +137,30 @@ module Alpha # # #### Query methods # - # * `isItemChecked(itemId: string): boolean`: Returns `true` if the item is checked, `false` otherwise. - # * `isItemHidden(itemId: string): boolean`: Returns `true` if the item is hidden, `false` otherwise. - # * `isItemDisabled(itemId: string): boolean`: Returns `true` if the item is disabled, `false` otherwise. - # * `getItemById(itemId: string): HTMLElement`: Returns the item's HTML `
  • ` element. + # * `getItemById(itemId: string): Element`: Returns the item's HTML `
  • ` element. The return value can be passed as the `item` argument to the other methods listed below. + # * `isItemChecked(item: Element): boolean`: Returns `true` if the item is checked, `false` otherwise. + # * `isItemHidden(item: Element): boolean`: Returns `true` if the item is hidden, `false` otherwise. + # * `isItemDisabled(item: Element): boolean`: Returns `true` if the item is disabled, `false` otherwise. # # #### State methods # - # * `showItemById(itemId: string)`: Shows the item, i.e. makes it visible. - # * `hideItemById(itemId: string)`: Hides the item, i.e. makes it invisible. - # * `enableItemById(itemId: string)`: Enables the item, i.e. makes it clickable by the mouse and keyboard. - # * `disableItemById(itemId: string)`: Disables the item, i.e. makes it unclickable by the mouse and keyboard. - # * `checkItemById(itemId: string)`: Checks the item. Only has an effect in single- and multi-select modes. - # * `uncheckItemById(itemId: string)`: Unchecks the item. Only has an effect in multi-select mode, since items cannot be unchecked in single-select mode. + # * `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. + # * `uncheckItem(item: Element)`: Unchecks the item. Only has an effect in multi-select mode, since items cannot be unchecked in single-select mode. + # + # #### Events + # + # The `` element fires an `itemActivated` event whenever an item is activated (eg. clicked) via the mouse or keyboard. + # + # ```typescript + # document.querySelector("action-menu").addEventListener("itemActivated", (event: ItemActivatedEvent) => { + # event.item // Element: the
  • item that was activated + # event.checked // boolean: whether or not the result of the activation checked the item + # }) + # ``` class ActionMenu < Primer::Component status :alpha 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 337ca72d9d..0608cb5779 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -12,6 +12,17 @@ type SelectedItem = { const validSelectors = ['[role="menuitem"]', '[role="menuitemcheckbox"]', '[role="menuitemradio"]'] const menuItemSelectors = validSelectors.map(selector => `:not([hidden]) > ${selector}`) +export type ItemActivatedEvent = { + item: Element + checked: boolean +} + +declare global { + interface HTMLElementEventMap { + itemActivated: CustomEvent + } +} + @controller export class ActionMenuElement extends HTMLElement { @target @@ -310,6 +321,11 @@ export class ActionMenuElement extends HTMLElement { } this.#updateInput() + this.dispatchEvent( + new CustomEvent('itemActivated', { + detail: {item: item.parentElement, checked: this.isItemChecked(item.parentElement)} + }) + ) } #activateItem(event: Event, item: Element) { @@ -416,7 +432,7 @@ export class ActionMenuElement extends HTMLElement { return this.querySelector(menuItemSelectors.join(',')) } - get #items(): HTMLElement[] { + get items(): HTMLElement[] { return Array.from(this.querySelectorAll(menuItemSelectors.join(','))) } @@ -424,9 +440,7 @@ export class ActionMenuElement extends HTMLElement { return this.querySelector(`li[data-item-id="${itemId}"`) } - isItemDisabled(itemId: string): boolean { - const item = this.getItemById(itemId) - + isItemDisabled(item: Element | null): boolean { if (item) { return item.classList.contains('ActionListItem--disabled') } else { @@ -434,27 +448,21 @@ export class ActionMenuElement extends HTMLElement { } } - disableItemById(itemId: string) { - const item = this.getItemById(itemId) - + disableItem(item: Element | null) { if (item) { item.classList.add('ActionListItem--disabled') item.querySelector('.ActionListContent')!.setAttribute('aria-disabled', 'true') } } - enableItemById(itemId: string) { - const item = this.getItemById(itemId) - + enableItem(item: Element | null) { if (item) { item.classList.remove('ActionListItem--disabled') item.querySelector('.ActionListContent')!.removeAttribute('aria-disabled') } } - isItemHidden(itemId: string): boolean { - const item = this.getItemById(itemId) - + isItemHidden(item: Element | null): boolean { if (item) { return item.hasAttribute('hidden') } else { @@ -462,25 +470,19 @@ export class ActionMenuElement extends HTMLElement { } } - hideItemById(itemId: string) { - const item = this.getItemById(itemId) - + hideItem(item: Element | null) { if (item) { item.setAttribute('hidden', 'hidden') } } - showItemById(itemId: string) { - const item = this.getItemById(itemId) - + showItem(item: Element | null) { if (item) { item.removeAttribute('hidden') } } - isItemChecked(itemId: string) { - const item = this.getItemById(itemId) - + isItemChecked(item: Element | null) { if (item) { return item.querySelector('.ActionListContent')!.getAttribute('aria-checked') === 'true' } else { @@ -488,9 +490,7 @@ export class ActionMenuElement extends HTMLElement { } } - checkItemById(itemId: string) { - const item = this.getItemById(itemId) - + checkItem(item: Element | null) { if (item && (this.selectVariant === 'single' || this.selectVariant === 'multiple')) { const itemContent = item.querySelector('.ActionListContent')! const ariaChecked = itemContent.getAttribute('aria-checked') === 'true' @@ -501,9 +501,7 @@ export class ActionMenuElement extends HTMLElement { } } - uncheckItemById(itemId: string) { - const item = this.getItemById(itemId) - + uncheckItem(item: Element | null) { if (item && (this.selectVariant === 'single' || this.selectVariant === 'multiple')) { const itemContent = item.querySelector('.ActionListContent')! const ariaChecked = itemContent.getAttribute('aria-checked') === 'true' diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index e95e70c9f3..c6c366f930 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -50,6 +50,12 @@ def assert_no_alert(message = nil, &block) # expected behavior end + def evaluate_multiline_script(script) + page.evaluate_script(<<~JS) + (() => { #{script} })() + JS + end + ########## TESTS ############ def test_dynamic_labels @@ -610,8 +616,9 @@ def test_unhidden_disabled_item_cannot_be_checked click_on_invoker_button refute_selector "li[data-item-id=hidden]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').showItemById('hidden'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.showItem(menu.getItemById('hidden')) JS assert_selector "li[data-item-id=hidden]" @@ -626,8 +633,9 @@ def test_hide_item_via_js_api click_on_invoker_button assert_selector "li[data-item-id=recursive]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').hideItemById('recursive'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.hideItem(menu.getItemById('recursive')); JS refute_selector "li[data-item-id=recursive]" @@ -639,8 +647,9 @@ def test_show_item_via_js_api click_on_invoker_button refute_selector "li[data-item-id=hidden]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').showItemById('hidden'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.showItem(menu.getItemById('hidden')) JS assert_selector "li[data-item-id=hidden]" @@ -652,8 +661,9 @@ def test_disable_item_via_js_api click_on_invoker_button refute_selector "li[data-item-id=resolve] [aria-disabled=true]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').disableItemById('resolve'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.disableItem(menu.getItemById('resolve')) JS assert_selector "li[data-item-id=resolve] [aria-disabled=true]" @@ -665,8 +675,9 @@ def test_enable_item_via_js_api click_on_invoker_button assert_selector "li[data-item-id=disabled] [aria-disabled=true]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').enableItemById('disabled'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.enableItem(menu.getItemById('disabled')) JS refute_selector "li[data-item-id=disabled] [aria-disabled=true]" @@ -678,8 +689,9 @@ def test_check_item_via_js_api click_on_invoker_button refute_selector "li[data-item-id=recursive] [aria-checked=true]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').checkItemById('recursive'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.checkItem(menu.getItemById('recursive')) JS click_on_invoker_button @@ -695,11 +707,32 @@ def test_uncheck_item_via_js_api assert_selector "li[data-item-id=jon] [aria-checked=true]" - page.evaluate_script(<<~JS) - document.querySelector('action-menu').uncheckItemById('jon'); + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.uncheckItem(menu.getItemById('jon')) JS refute_selector "li[data-item-id=jon] [aria-checked=true]" end + + def test_fires_event_on_activation + visit_preview(:single_select) + + evaluate_multiline_script(<<~JS) + window.activatedItemText = null + window.activatedItemChecked = false + + document.querySelector('action-menu').addEventListener('itemActivated', (event) => { + window.activatedItemText = event.detail.item.innerText + window.activatedItemChecked = event.detail.checked + }) + JS + + click_on_invoker_button + click_on_first_item + + assert page.evaluate_script("window.activatedItemChecked") + assert_equal "Fast forward", page.evaluate_script("window.activatedItemText") + end end end From 86ec140276813e1059d18f221fbd7ed0d8d06311 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 22 Nov 2023 14:46:22 -0800 Subject: [PATCH 5/6] Try bumping prettier --- package-lock.json | 74 ++++++++++++++--------------------------------- package.json | 2 +- 2 files changed, 22 insertions(+), 54 deletions(-) diff --git a/package-lock.json b/package-lock.json index 82f3cae5a9..d2eba4b55b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,7 +42,7 @@ "eslint": "^8.23.1", "eslint-plugin-custom-elements": "^0.0.6", "eslint-plugin-github": "^4.9.2", - "eslint-plugin-prettier": "^4.2.1", + "eslint-plugin-prettier": "^5.0.0", "markdownlint-cli2": "^0.10.0", "mocha": "^10.0.0", "playwright": "^1.35.1", @@ -4913,35 +4913,6 @@ "eslint": "^8.0.1" } }, - "node_modules/eslint-plugin-github/node_modules/eslint-plugin-prettier": { - "version": "5.0.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", - "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", - "dev": true, - "dependencies": { - "prettier-linter-helpers": "^1.0.0", - "synckit": "^0.8.5" - }, - "engines": { - "node": "^14.18.0 || >=16.0.0" - }, - "funding": { - "url": "https://opencollective.com/prettier" - }, - "peerDependencies": { - "@types/eslint": ">=8.0.0", - "eslint": ">=8.0.0", - "prettier": ">=3.0.0" - }, - "peerDependenciesMeta": { - "@types/eslint": { - "optional": true - }, - "eslint-config-prettier": { - "optional": true - } - } - }, "node_modules/eslint-plugin-i18n-text": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/eslint-plugin-i18n-text/-/eslint-plugin-i18n-text-1.0.1.tgz", @@ -5059,21 +5030,29 @@ } }, "node_modules/eslint-plugin-prettier": { - "version": "4.2.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-4.2.1.tgz", - "integrity": "sha512-f/0rXLXUt0oFYs8ra4w49wYZBG5GKZpAYsJSm6rnYL5uVDjd+zowwMwVZHnAjf4edNrKpCDYfXDgmRE/Ak7QyQ==", + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", + "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", "dev": true, "dependencies": { - "prettier-linter-helpers": "^1.0.0" + "prettier-linter-helpers": "^1.0.0", + "synckit": "^0.8.5" }, "engines": { - "node": ">=12.0.0" + "node": "^14.18.0 || >=16.0.0" + }, + "funding": { + "url": "https://opencollective.com/prettier" }, "peerDependencies": { - "eslint": ">=7.28.0", - "prettier": ">=2.0.0" + "@types/eslint": ">=8.0.0", + "eslint": ">=8.0.0", + "prettier": ">=3.0.0" }, "peerDependenciesMeta": { + "@types/eslint": { + "optional": true + }, "eslint-config-prettier": { "optional": true } @@ -15187,18 +15166,6 @@ "jsx-ast-utils": "^3.3.2", "prettier": "^3.0.0", "svg-element-attributes": "^1.3.1" - }, - "dependencies": { - "eslint-plugin-prettier": { - "version": "5.0.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", - "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", - "dev": true, - "requires": { - "prettier-linter-helpers": "^1.0.0", - "synckit": "^0.8.5" - } - } } }, "eslint-plugin-i18n-text": { @@ -15296,12 +15263,13 @@ "dev": true }, "eslint-plugin-prettier": { - "version": "4.2.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-4.2.1.tgz", - "integrity": "sha512-f/0rXLXUt0oFYs8ra4w49wYZBG5GKZpAYsJSm6rnYL5uVDjd+zowwMwVZHnAjf4edNrKpCDYfXDgmRE/Ak7QyQ==", + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", + "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", "dev": true, "requires": { - "prettier-linter-helpers": "^1.0.0" + "prettier-linter-helpers": "^1.0.0", + "synckit": "^0.8.5" } }, "eslint-rule-documentation": { diff --git a/package.json b/package.json index 3362a32d02..9443853233 100644 --- a/package.json +++ b/package.json @@ -75,7 +75,7 @@ "eslint": "^8.23.1", "eslint-plugin-custom-elements": "^0.0.6", "eslint-plugin-github": "^4.9.2", - "eslint-plugin-prettier": "^4.2.1", + "eslint-plugin-prettier": "^5.0.0", "markdownlint-cli2": "^0.10.0", "mocha": "^10.0.0", "playwright": "^1.35.1", From 47d06901d0b3eba8153a76976f59eda92a3bece0 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 22 Nov 2023 14:49:41 -0800 Subject: [PATCH 6/6] Fix linting --- .../primer/alpha/action_menu/action_menu_element.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 0608cb5779..ba4caffaaa 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -93,7 +93,7 @@ export class ActionMenuElement extends HTMLElement { results.push({ label: labelEl?.textContent, value: selectedItem?.getAttribute('data-value'), - element: selectedItem + element: selectedItem, }) } @@ -113,7 +113,7 @@ export class ActionMenuElement extends HTMLElement { if (this.includeFragment) { this.includeFragment.addEventListener('include-fragment-replaced', this, { - signal + signal, }) } } @@ -323,8 +323,8 @@ export class ActionMenuElement extends HTMLElement { this.#updateInput() this.dispatchEvent( new CustomEvent('itemActivated', { - detail: {item: item.parentElement, checked: this.isItemChecked(item.parentElement)} - }) + detail: {item: item.parentElement, checked: this.isItemChecked(item.parentElement)}, + }), ) }