From 20b79f10e13af07299c7a6dcee271d38939546f9 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Thu, 9 Feb 2023 16:07:37 -0800 Subject: [PATCH 1/6] Allow NavList selection/deselection by id/href in javascript --- .../primer/alpha/action_list/item.rb | 3 + app/components/primer/alpha/nav_list.ts | 102 ++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/app/components/primer/alpha/action_list/item.rb b/app/components/primer/alpha/action_list/item.rb index 5b92a882b8..f7a7d7af45 100644 --- a/app/components/primer/alpha/action_list/item.rb +++ b/app/components/primer/alpha/action_list/item.rb @@ -187,6 +187,9 @@ def initialize( @system_arguments[:aria] ||= {} @system_arguments[:aria][:disabled] = "true" if @disabled + @system_arguments[:data] ||= {} + @system_arguments[:data][:targets] = "#{list.custom_element_name}.items" + @label_arguments = { classes: class_names( label_classes, diff --git a/app/components/primer/alpha/nav_list.ts b/app/components/primer/alpha/nav_list.ts index c89b5a98c0..23825db4f9 100644 --- a/app/components/primer/alpha/nav_list.ts +++ b/app/components/primer/alpha/nav_list.ts @@ -4,6 +4,7 @@ import {controller, target, targets} from '@github/catalyst' @controller class NavListElement extends HTMLElement { @target list: HTMLElement + @targets items: HTMLElement[] @target showMoreItem: HTMLElement @targets focusMarkers: HTMLElement[] @@ -40,6 +41,43 @@ class NavListElement extends HTMLElement { return this.showMoreItem.getAttribute('src') || '' } + selectItemById(itemId: string | null): boolean { + if (!itemId) return false + + const selectedItem = this.findSelectedNavItemById(itemId) + + if (selectedItem) { + this.select(selectedItem) + return true + } + + return false + } + + selectItemByHref(href: string | null): boolean { + if (!href) return false + + const selectedItem = this.findSelectedNavItemByHref(href) + + if (selectedItem) { + this.select(selectedItem) + return true + } + + return false + } + + selectItemByCurrentLocation(): boolean { + const selectedItem = this.findSelectedNavItemByCurrentLocation() + + if (selectedItem) { + this.select(selectedItem) + return true + } + + return false + } + // expand collapsible item onClick expandItem(item: HTMLElement) { item.nextElementSibling?.removeAttribute('data-hidden') @@ -120,6 +158,70 @@ class NavListElement extends HTMLElement { template.innerHTML = html return document.importNode(template.content, true) } + + private findSelectedNavItemById(itemId: string): HTMLElement | null { + // First we compare the selected link to data-item-id for each nav item + for (const navItem of this.items) { + const keys = navItem.getAttribute('data-item-id')?.split(' ') || [] + + if (keys.includes(itemId)) { + return navItem + } + } + + return null + } + + private findSelectedNavItemByHref(href: string): HTMLElement | null { + // If we didn't find a match, we compare the selected link to the href of each nav item + const selectedNavItem = this.querySelector(`.ActionListContent[href="${href}"]`) + if (selectedNavItem) { + return selectedNavItem.closest('.ActionListItem') + } + + return null + } + + private findSelectedNavItemByCurrentLocation(): HTMLElement | null { + return this.findSelectedNavItemByHref(window.location.pathname) + } + + private select(navItem: HTMLElement) { + const currentlySelectedItem = this.querySelector('.ActionListItem--navActive') as HTMLElement + if (currentlySelectedItem) this.deselect(currentlySelectedItem) + + navItem.classList.add('ActionListItem--navActive') + + const parentMenu = this.findParentMenu(navItem) + + if (parentMenu) { + this.expandItem(parentMenu) + parentMenu.classList.add('ActionListContent--hasActiveSubItem') + } + } + + private deselect(navItem: HTMLElement) { + navItem.classList.remove('ActionListItem--navActive') + + const parentMenu = this.findParentMenu(navItem) + + if (parentMenu) { + this.collapseItem(parentMenu) + parentMenu.classList.remove('ActionListContent--hasActiveSubItem') + } + } + + private findParentMenu(navItem: HTMLElement): HTMLElement | null { + if (!navItem.classList.contains('ActionListItem--subItem')) return null + + const parent = navItem.parentElement?.parentElement?.querySelector('button.ActionListContent') + + if (parent) { + return parent as HTMLElement + } else { + return null + } + } } declare global { From 0396017263af310cc7892bfd9c08080519d08d55 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Thu, 9 Feb 2023 21:34:41 -0800 Subject: [PATCH 2/6] Export NavListElement --- app/components/primer/alpha/nav_list.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/primer/alpha/nav_list.ts b/app/components/primer/alpha/nav_list.ts index 23825db4f9..afc68c45c9 100644 --- a/app/components/primer/alpha/nav_list.ts +++ b/app/components/primer/alpha/nav_list.ts @@ -2,7 +2,7 @@ import {controller, target, targets} from '@github/catalyst' @controller -class NavListElement extends HTMLElement { +export class NavListElement extends HTMLElement { @target list: HTMLElement @targets items: HTMLElement[] @target showMoreItem: HTMLElement From 7efd3f669458c3533d4c428264a6c8b147742119 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 10 Feb 2023 11:19:59 -0800 Subject: [PATCH 3/6] Add tests --- .../primer/alpha/action_list/item.rb | 6 +- app/components/primer/alpha/nav_list.rb | 5 + app/components/primer/alpha/nav_list/item.rb | 4 + .../primer/alpha/nav_list/section.rb | 2 +- test/system/alpha/nav_list_test.rb | 98 +++++++++++++++++++ test/system/test_case.rb | 2 +- test/test_helpers/cuprite_setup.rb | 2 +- 7 files changed, 115 insertions(+), 4 deletions(-) diff --git a/app/components/primer/alpha/action_list/item.rb b/app/components/primer/alpha/action_list/item.rb index f7a7d7af45..e444e5cd3b 100644 --- a/app/components/primer/alpha/action_list/item.rb +++ b/app/components/primer/alpha/action_list/item.rb @@ -188,7 +188,7 @@ def initialize( @system_arguments[:aria][:disabled] = "true" if @disabled @system_arguments[:data] ||= {} - @system_arguments[:data][:targets] = "#{list.custom_element_name}.items" + @system_arguments[:data][:targets] = "#{list_class.custom_element_name}.items" @label_arguments = { classes: class_names( @@ -239,6 +239,10 @@ def before_render "ActionListContent--blockDescription" => description && @description_scheme == :block ) end + + def list_class + Primer::Alpha::ActionList + end end end end diff --git a/app/components/primer/alpha/nav_list.rb b/app/components/primer/alpha/nav_list.rb index 09f4c82d6b..069695c778 100644 --- a/app/components/primer/alpha/nav_list.rb +++ b/app/components/primer/alpha/nav_list.rb @@ -17,6 +17,11 @@ module Alpha class NavList < Primer::Component status :alpha + # @private + def self.custom_element_name + "nav-list" + end + # Sections. Each section is a list of links and an optional heading. # # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::NavList::Section) %>. diff --git a/app/components/primer/alpha/nav_list/item.rb b/app/components/primer/alpha/nav_list/item.rb index e1691be77e..669b34e469 100644 --- a/app/components/primer/alpha/nav_list/item.rb +++ b/app/components/primer/alpha/nav_list/item.rb @@ -119,6 +119,10 @@ def active_sub_item? def current_page?(url) helpers.current_page?(url) end + + def list_class + Primer::Alpha::NavList + end end end end diff --git a/app/components/primer/alpha/nav_list/section.rb b/app/components/primer/alpha/nav_list/section.rb index eeff24754e..c8824d2a80 100644 --- a/app/components/primer/alpha/nav_list/section.rb +++ b/app/components/primer/alpha/nav_list/section.rb @@ -38,7 +38,7 @@ class Section < ActionList # @private def self.custom_element_name - "nav-list" + Primer::Alpha::NavList.custom_element_name end # @param selected_item_id [Symbol] The ID of the currently selected item. Used internally. diff --git a/test/system/alpha/nav_list_test.rb b/test/system/alpha/nav_list_test.rb index 6b97259b38..aaa336c375 100644 --- a/test/system/alpha/nav_list_test.rb +++ b/test/system/alpha/nav_list_test.rb @@ -29,5 +29,103 @@ def test_shows_more_items assert_selector "li", text: "Bachelor Chow" assert_selector "li", text: "LöBrau" end + + def test_js_api_allows_selecting_item_by_id + visit_preview(:default) + + select_item_by_id(:collaborators) + assert_item_id_selected(:collaborators) + end + + def test_selecting_id_collapses_section + visit_preview(:default) + + select_item_by_id(:collaborators) + assert_selector "button[aria-expanded=false]", text: "Moderation" + end + + def test_js_api_allows_selecting_item_by_href + visit_preview(:default) + + select_item_by_href("/collaborators") + assert_item_href_selected("/collaborators") + end + + def test_selecting_href_collapses_section + visit_preview(:default) + + select_item_by_href("/collaborators") + assert_selector "button[aria-expanded=false]", text: "Moderation" + end + + def test_js_api_allows_selecting_item_by_current_location + visit_preview(:default) + + # set the URL without reloading the page + page.evaluate_script(<<~JS) + (() => { + window.history.pushState({}, "", "http://localhost/collaborators") + })(); + JS + + select_item_by_current_location + assert_item_href_selected("/collaborators") + assert_selector "button[aria-expanded=false]", text: "Moderation" + end + + private + + def select_item_by_id(id) + select_item_by(id: id) + end + + def select_item_by_href(href) + select_item_by(href: href) + end + + def select_item_by_current_location + page.evaluate_script(<<~JS) + (() => { + const navLists = document.querySelectorAll('nav-list'); + + // Unfortunately the NavList component emits multiple elements, + // so we have to loop over all of them. This will be addressed in the very + // near future. + for (const navList of navLists) { + if (navList.selectItemByCurrentLocation()) { + return; + } + } + })(); + JS + end + + def select_item_by(id: nil, href: nil) + func = id ? "selectItemById" : "selectItemByHref" + param = id || href + + page.evaluate_script(<<~JS) + (() => { + const navLists = document.querySelectorAll('nav-list'); + + // Unfortunately the NavList component emits multiple elements, + // so we have to loop over all of them. This will be addressed in the very + // near future. + for (const navList of navLists) { + if (navList.#{func}('#{param}')) { + return; + } + } + })(); + JS + end + + def assert_item_id_selected(item_id) + assert_selector("li[data-item-id='#{item_id}'].ActionListItem--navActive") + end + + def assert_item_href_selected(item_href) + assert_selector("li.ActionListItem--navActive a[href='#{item_href}']") + end end end diff --git a/test/system/test_case.rb b/test/system/test_case.rb index 1ad73e6d37..3edb07f7b9 100644 --- a/test/system/test_case.rb +++ b/test/system/test_case.rb @@ -9,7 +9,7 @@ module System class TestCase < ActionDispatch::SystemTestCase - driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: { process_timeout: 240, timeout: 240 } + driven_by :primer_cuprite, using: :chrome, screen_size: [1400, 1400], options: { process_timeout: 240, timeout: 240 } # Skip `:region` which relates to preview page structure rather than individual component. # Skip `:color-contrast` which requires primer design-level change. diff --git a/test/test_helpers/cuprite_setup.rb b/test/test_helpers/cuprite_setup.rb index 6ab88b9e39..e48e36267e 100644 --- a/test/test_helpers/cuprite_setup.rb +++ b/test/test_helpers/cuprite_setup.rb @@ -5,7 +5,7 @@ # Then, we need to register our driver to be able to use it later # with #driven_by method. -Capybara.register_driver(:cuprite) do |app| +Capybara.register_driver(:primer_cuprite) do |app| Capybara::Cuprite::Driver.new( app, **{ From 5a08664a511f4f5566b6c9dd55c5e6bf60f365c2 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 10 Feb 2023 11:19:59 -0800 Subject: [PATCH 4/6] Add tests --- .../primer/alpha/action_list/item.rb | 6 +- app/components/primer/alpha/nav_list.rb | 5 + app/components/primer/alpha/nav_list/item.rb | 4 + .../primer/alpha/nav_list/section.rb | 2 +- test/system/alpha/nav_list_test.rb | 98 +++++++++++++++++++ test/system/test_case.rb | 2 +- test/test_helpers/cuprite_setup.rb | 2 +- 7 files changed, 115 insertions(+), 4 deletions(-) diff --git a/app/components/primer/alpha/action_list/item.rb b/app/components/primer/alpha/action_list/item.rb index f7a7d7af45..e444e5cd3b 100644 --- a/app/components/primer/alpha/action_list/item.rb +++ b/app/components/primer/alpha/action_list/item.rb @@ -188,7 +188,7 @@ def initialize( @system_arguments[:aria][:disabled] = "true" if @disabled @system_arguments[:data] ||= {} - @system_arguments[:data][:targets] = "#{list.custom_element_name}.items" + @system_arguments[:data][:targets] = "#{list_class.custom_element_name}.items" @label_arguments = { classes: class_names( @@ -239,6 +239,10 @@ def before_render "ActionListContent--blockDescription" => description && @description_scheme == :block ) end + + def list_class + Primer::Alpha::ActionList + end end end end diff --git a/app/components/primer/alpha/nav_list.rb b/app/components/primer/alpha/nav_list.rb index 09f4c82d6b..069695c778 100644 --- a/app/components/primer/alpha/nav_list.rb +++ b/app/components/primer/alpha/nav_list.rb @@ -17,6 +17,11 @@ module Alpha class NavList < Primer::Component status :alpha + # @private + def self.custom_element_name + "nav-list" + end + # Sections. Each section is a list of links and an optional heading. # # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::NavList::Section) %>. diff --git a/app/components/primer/alpha/nav_list/item.rb b/app/components/primer/alpha/nav_list/item.rb index e1691be77e..669b34e469 100644 --- a/app/components/primer/alpha/nav_list/item.rb +++ b/app/components/primer/alpha/nav_list/item.rb @@ -119,6 +119,10 @@ def active_sub_item? def current_page?(url) helpers.current_page?(url) end + + def list_class + Primer::Alpha::NavList + end end end end diff --git a/app/components/primer/alpha/nav_list/section.rb b/app/components/primer/alpha/nav_list/section.rb index eeff24754e..c8824d2a80 100644 --- a/app/components/primer/alpha/nav_list/section.rb +++ b/app/components/primer/alpha/nav_list/section.rb @@ -38,7 +38,7 @@ class Section < ActionList # @private def self.custom_element_name - "nav-list" + Primer::Alpha::NavList.custom_element_name end # @param selected_item_id [Symbol] The ID of the currently selected item. Used internally. diff --git a/test/system/alpha/nav_list_test.rb b/test/system/alpha/nav_list_test.rb index 6b97259b38..aaa336c375 100644 --- a/test/system/alpha/nav_list_test.rb +++ b/test/system/alpha/nav_list_test.rb @@ -29,5 +29,103 @@ def test_shows_more_items assert_selector "li", text: "Bachelor Chow" assert_selector "li", text: "LöBrau" end + + def test_js_api_allows_selecting_item_by_id + visit_preview(:default) + + select_item_by_id(:collaborators) + assert_item_id_selected(:collaborators) + end + + def test_selecting_id_collapses_section + visit_preview(:default) + + select_item_by_id(:collaborators) + assert_selector "button[aria-expanded=false]", text: "Moderation" + end + + def test_js_api_allows_selecting_item_by_href + visit_preview(:default) + + select_item_by_href("/collaborators") + assert_item_href_selected("/collaborators") + end + + def test_selecting_href_collapses_section + visit_preview(:default) + + select_item_by_href("/collaborators") + assert_selector "button[aria-expanded=false]", text: "Moderation" + end + + def test_js_api_allows_selecting_item_by_current_location + visit_preview(:default) + + # set the URL without reloading the page + page.evaluate_script(<<~JS) + (() => { + window.history.pushState({}, "", "http://localhost/collaborators") + })(); + JS + + select_item_by_current_location + assert_item_href_selected("/collaborators") + assert_selector "button[aria-expanded=false]", text: "Moderation" + end + + private + + def select_item_by_id(id) + select_item_by(id: id) + end + + def select_item_by_href(href) + select_item_by(href: href) + end + + def select_item_by_current_location + page.evaluate_script(<<~JS) + (() => { + const navLists = document.querySelectorAll('nav-list'); + + // Unfortunately the NavList component emits multiple elements, + // so we have to loop over all of them. This will be addressed in the very + // near future. + for (const navList of navLists) { + if (navList.selectItemByCurrentLocation()) { + return; + } + } + })(); + JS + end + + def select_item_by(id: nil, href: nil) + func = id ? "selectItemById" : "selectItemByHref" + param = id || href + + page.evaluate_script(<<~JS) + (() => { + const navLists = document.querySelectorAll('nav-list'); + + // Unfortunately the NavList component emits multiple elements, + // so we have to loop over all of them. This will be addressed in the very + // near future. + for (const navList of navLists) { + if (navList.#{func}('#{param}')) { + return; + } + } + })(); + JS + end + + def assert_item_id_selected(item_id) + assert_selector("li[data-item-id='#{item_id}'].ActionListItem--navActive") + end + + def assert_item_href_selected(item_href) + assert_selector("li.ActionListItem--navActive a[href='#{item_href}']") + end end end diff --git a/test/system/test_case.rb b/test/system/test_case.rb index 1ad73e6d37..3edb07f7b9 100644 --- a/test/system/test_case.rb +++ b/test/system/test_case.rb @@ -9,7 +9,7 @@ module System class TestCase < ActionDispatch::SystemTestCase - driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: { process_timeout: 240, timeout: 240 } + driven_by :primer_cuprite, using: :chrome, screen_size: [1400, 1400], options: { process_timeout: 240, timeout: 240 } # Skip `:region` which relates to preview page structure rather than individual component. # Skip `:color-contrast` which requires primer design-level change. diff --git a/test/test_helpers/cuprite_setup.rb b/test/test_helpers/cuprite_setup.rb index 6ab88b9e39..e48e36267e 100644 --- a/test/test_helpers/cuprite_setup.rb +++ b/test/test_helpers/cuprite_setup.rb @@ -5,7 +5,7 @@ # Then, we need to register our driver to be able to use it later # with #driven_by method. -Capybara.register_driver(:cuprite) do |app| +Capybara.register_driver(:primer_cuprite) do |app| Capybara::Cuprite::Driver.new( app, **{ From 852bd6122f0e9cf2294077be795bbd55c08a178f Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 10 Feb 2023 11:25:11 -0800 Subject: [PATCH 5/6] Add changeset --- .changeset/tender-islands-peel.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tender-islands-peel.md diff --git a/.changeset/tender-islands-peel.md b/.changeset/tender-islands-peel.md new file mode 100644 index 0000000000..e19b199673 --- /dev/null +++ b/.changeset/tender-islands-peel.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Allow NavList selection/deselection by id/href in javascript From f3fba2eceb72cdaebe0b6903df1faf9bb53fc4f7 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 15 Feb 2023 15:19:04 -0800 Subject: [PATCH 6/6] Address PR feedback --- app/components/primer/alpha/nav_list.ts | 38 ++++++++++++------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/app/components/primer/alpha/nav_list.ts b/app/components/primer/alpha/nav_list.ts index afc68c45c9..10bcd07036 100644 --- a/app/components/primer/alpha/nav_list.ts +++ b/app/components/primer/alpha/nav_list.ts @@ -44,10 +44,10 @@ export class NavListElement extends HTMLElement { selectItemById(itemId: string | null): boolean { if (!itemId) return false - const selectedItem = this.findSelectedNavItemById(itemId) + const selectedItem = this.#findSelectedNavItemById(itemId) if (selectedItem) { - this.select(selectedItem) + this.#select(selectedItem) return true } @@ -57,10 +57,10 @@ export class NavListElement extends HTMLElement { selectItemByHref(href: string | null): boolean { if (!href) return false - const selectedItem = this.findSelectedNavItemByHref(href) + const selectedItem = this.#findSelectedNavItemByHref(href) if (selectedItem) { - this.select(selectedItem) + this.#select(selectedItem) return true } @@ -68,10 +68,10 @@ export class NavListElement extends HTMLElement { } selectItemByCurrentLocation(): boolean { - const selectedItem = this.findSelectedNavItemByCurrentLocation() + const selectedItem = this.#findSelectedNavItemByCurrentLocation() if (selectedItem) { - this.select(selectedItem) + this.#select(selectedItem) return true } @@ -133,7 +133,7 @@ export class NavListElement extends HTMLElement { this.currentPage-- return } - const fragment = this.parseHTML(document, html) + const fragment = this.#parseHTML(document, html) fragment?.querySelector('li > a')?.setAttribute('data-targets', 'nav-list.focusMarkers') this.list.insertBefore(fragment, this.showMoreItem) this.focusMarkers.pop()?.focus() @@ -152,14 +152,14 @@ export class NavListElement extends HTMLElement { } } - private parseHTML(document: Document, html: string): DocumentFragment { + #parseHTML(document: Document, html: string): DocumentFragment { const template = document.createElement('template') // eslint-disable-next-line github/no-inner-html template.innerHTML = html return document.importNode(template.content, true) } - private findSelectedNavItemById(itemId: string): HTMLElement | null { + #findSelectedNavItemById(itemId: string): HTMLElement | null { // First we compare the selected link to data-item-id for each nav item for (const navItem of this.items) { const keys = navItem.getAttribute('data-item-id')?.split(' ') || [] @@ -172,7 +172,7 @@ export class NavListElement extends HTMLElement { return null } - private findSelectedNavItemByHref(href: string): HTMLElement | null { + #findSelectedNavItemByHref(href: string): HTMLElement | null { // If we didn't find a match, we compare the selected link to the href of each nav item const selectedNavItem = this.querySelector(`.ActionListContent[href="${href}"]`) if (selectedNavItem) { @@ -182,17 +182,17 @@ export class NavListElement extends HTMLElement { return null } - private findSelectedNavItemByCurrentLocation(): HTMLElement | null { - return this.findSelectedNavItemByHref(window.location.pathname) + #findSelectedNavItemByCurrentLocation(): HTMLElement | null { + return this.#findSelectedNavItemByHref(window.location.pathname) } - private select(navItem: HTMLElement) { + #select(navItem: HTMLElement) { const currentlySelectedItem = this.querySelector('.ActionListItem--navActive') as HTMLElement - if (currentlySelectedItem) this.deselect(currentlySelectedItem) + if (currentlySelectedItem) this.#deselect(currentlySelectedItem) navItem.classList.add('ActionListItem--navActive') - const parentMenu = this.findParentMenu(navItem) + const parentMenu = this.#findParentMenu(navItem) if (parentMenu) { this.expandItem(parentMenu) @@ -200,10 +200,10 @@ export class NavListElement extends HTMLElement { } } - private deselect(navItem: HTMLElement) { + #deselect(navItem: HTMLElement) { navItem.classList.remove('ActionListItem--navActive') - const parentMenu = this.findParentMenu(navItem) + const parentMenu = this.#findParentMenu(navItem) if (parentMenu) { this.collapseItem(parentMenu) @@ -211,10 +211,10 @@ export class NavListElement extends HTMLElement { } } - private findParentMenu(navItem: HTMLElement): HTMLElement | null { + #findParentMenu(navItem: HTMLElement): HTMLElement | null { if (!navItem.classList.contains('ActionListItem--subItem')) return null - const parent = navItem.parentElement?.parentElement?.querySelector('button.ActionListContent') + const parent = navItem.closest('li.ActionListItem--hasSubItem')?.querySelector('button.ActionListContent') if (parent) { return parent as HTMLElement