diff --git a/.changeset/dull-knives-leave.md b/.changeset/dull-knives-leave.md new file mode 100644 index 0000000000..f93f919438 --- /dev/null +++ b/.changeset/dull-knives-leave.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Fixes Alpha::SelectPanel remote loading bug where items from the server were overriding the users selections. Additionally, update the #removeSelectedItem function to grab the data-value from the correct element. diff --git a/app/components/primer/alpha/select_panel_element.ts b/app/components/primer/alpha/select_panel_element.ts index f2e5beed82..1774837d8a 100644 --- a/app/components/primer/alpha/select_panel_element.ts +++ b/app/components/primer/alpha/select_panel_element.ts @@ -84,6 +84,7 @@ export class SelectPanelElement extends HTMLElement { #selectedItems: Map = new Map() #loadingDelayTimeoutId: number | null = null #loadingAnnouncementTimeoutId: number | null = null + #hasLoadedData = false get open(): boolean { return this.dialog.open @@ -399,9 +400,11 @@ export class SelectPanelElement extends HTMLElement { } } - #removeSelectedItem(item: Element) { - const value = item.getAttribute('data-value') + #removeSelectedItem(item: SelectPanelItem) { + const itemContent = this.#getItemContent(item) + if (!itemContent) return + const value = itemContent.getAttribute('data-value') if (value) { this.#selectedItems.delete(value) } @@ -700,8 +703,12 @@ export class SelectPanelElement extends HTMLElement { if (!itemContent) continue const value = itemContent.getAttribute('data-value') - - if (value && !this.#selectedItems.has(value) && this.isItemChecked(item)) { + if (this.#hasLoadedData) { + if (value && !this.#selectedItems.has(value)) { + itemContent.setAttribute(this.ariaSelectionType, 'false') + } + } else if (value && !this.#selectedItems.has(value) && this.isItemChecked(item)) { + this.#hasLoadedData = true this.#addSelectedItem(item) } } @@ -863,19 +870,18 @@ export class SelectPanelElement extends HTMLElement { const itemContent = this.#getItemContent(item) if (this.selectVariant === 'single') { + const element = this.selectedItems[0]?.element + if (element) { + this.#getItemContent(element)?.setAttribute(this.ariaSelectionType, 'false') + } + this.#selectedItems.clear() + // Only check, never uncheck here. Single-select mode does not allow unchecking a checked item. if (checked) { this.#addSelectedItem(item) itemContent?.setAttribute(this.ariaSelectionType, 'true') } - for (const checkedItem of this.querySelectorAll(`[${this.ariaSelectionType}]`)) { - if (checkedItem !== itemContent) { - this.#removeSelectedItem(checkedItem) - checkedItem.setAttribute(this.ariaSelectionType, 'false') - } - } - this.#setDynamicLabel() } else { // multi-select mode allows unchecking a checked item diff --git a/demo/app/controllers/select_panel_items_controller.rb b/demo/app/controllers/select_panel_items_controller.rb index 7ad493e77f..38a4433ac3 100644 --- a/demo/app/controllers/select_panel_items_controller.rb +++ b/demo/app/controllers/select_panel_items_controller.rb @@ -42,6 +42,12 @@ def index [] end + if unselect_items? + results.map! do |result| + result.merge(selected: false) + end + end + clean_up_old_uuids(uuid) respond_to do |format| @@ -84,4 +90,12 @@ def clean_up_old_uuids(current_uuid) def key_for(uuid) "#{COOKIE_PREFIX}#{uuid}" end + + def select_items? + params.fetch(:select_items, "true") == "true" + end + + def unselect_items? + !select_items? + end end diff --git a/previews/primer/alpha/select_panel_preview.rb b/previews/primer/alpha/select_panel_preview.rb index 791897a2c9..4e9049dcba 100644 --- a/previews/primer/alpha/select_panel_preview.rb +++ b/previews/primer/alpha/select_panel_preview.rb @@ -18,6 +18,7 @@ class SelectPanelPreview < ViewComponent::Preview # @param open_on_load toggle # @param anchor_align [Symbol] select [start, center, end] # @param anchor_side [Symbol] select [outside_bottom, outside_top, outside_left, outside_right] + # @param select_items toggle def playground( title: "Sci-fi equipment", subtitle: "Various tools from your favorite shows", @@ -31,10 +32,12 @@ def playground( show_filter: true, open_on_load: false, anchor_align: :start, - anchor_side: :outside_bottom + anchor_side: :outside_bottom, + select_items: true ) render_with_template(locals: { subtitle: subtitle, + select_items: select_items, system_arguments: { title: title, size: size, diff --git a/previews/primer/alpha/select_panel_preview/playground.html.erb b/previews/primer/alpha/select_panel_preview/playground.html.erb index f48ef4ff81..ed10c36d7c 100644 --- a/previews/primer/alpha/select_panel_preview/playground.html.erb +++ b/previews/primer/alpha/select_panel_preview/playground.html.erb @@ -8,7 +8,8 @@ src: select_panel_items_path( select_variant: :single, show_results: !simulate_no_results, - fail: simulate_failure + fail: simulate_failure, + select_items: select_items ), select_variant: :single, fetch_strategy: :remote, diff --git a/test/system/alpha/select_panel_test.rb b/test/system/alpha/select_panel_test.rb index ef719a404b..a46a39f2e0 100644 --- a/test/system/alpha/select_panel_test.rb +++ b/test/system/alpha/select_panel_test.rb @@ -1,3 +1,4 @@ + # frozen_string_literal: true require "system/test_case" @@ -182,11 +183,12 @@ def test_remembers_selections_on_filter end # Phaser should already be selected - assert_selector "[aria-checked=true]", count: 1 + assert_selector "[aria-checked=true]", text: "Phaser" click_on "Photon torpedo" - assert_selector "[aria-checked=true]", count: 2 + assert_selector "[aria-checked=true]", text: "Phaser" + assert_selector "[aria-checked=true]", text: "Photon torpedo" wait_for_items_to_load do filter_results(query: "ph") @@ -382,6 +384,100 @@ def test_single_select_disabled_item_cannot_be_checked refute_selector "[aria-selected=true]" end + def test_single_select_does_not_allow_server_to_check_items_on_filter_if_selections_already_made + # playground is single-select + visit_preview(:playground) + + wait_for_items_to_load do + click_on_invoker_button + end + + # Phaser should already be selected + assert_selector "[aria-selected=true]", text: "Phaser" + + click_on "Photon torpedo" + click_on_invoker_button + + wait_for_items_to_load do + filter_results(query: "ph") + end + + # server will render this item checked, but since the user has already made selections, + # the server-rendered selections should be ignored + refute_selector "[aria-selected=true]", text: "Phaser" + assert_selector "[aria-selected=true]", text: "Photon torpedo" + end + + def test_single_select_remembers_only_one_checked_item_and_ignores_checked_items_from_server + # playground is single-select + visit_preview(:playground) + + wait_for_items_to_load do + click_on_invoker_button + end + + # Phaser should already be selected + assert_selector "[aria-selected=true]", text: "Phaser" + + wait_for_items_to_load do + filter_results(query: "light") + end + + click_on "Lightsaber" + + click_on_invoker_button + + wait_for_items_to_load do + filter_results(query: "") + end + + refute_selector "[aria-selected=true]", text: "Phaser" + assert_selector "[aria-selected=true]", text: "Lightsaber" + end + + def test_single_select_handles_all_options_unselected_by_default + # playground is single-select + visit_preview(:playground, select_items: false) + + wait_for_items_to_load do + click_on_invoker_button + end + + # Phaser should note already be selected + refute_selector "[aria-selected=true]", text: "Phaser" + + wait_for_items_to_load do + filter_results(query: "light") + end + + click_on "Lightsaber" + + click_on_invoker_button + + wait_for_items_to_load do + filter_results(query: "") + end + + refute_selector "[aria-selected=true]", text: "Phaser" + assert_selector "[aria-selected=true]", text: "Lightsaber" + + + wait_for_items_to_load do + filter_results(query: "ph") + end + + click_on "Phaser" + + click_on_invoker_button + + wait_for_items_to_load do + filter_results(query: "") + end + + assert_selector "[aria-selected=true]", text: "Phaser" + refute_selector "[aria-selected=true]", text: "Lightsaber" + end + ########## MULTISELECT TESTS ############ def test_multi_select_items_checked @@ -477,6 +573,31 @@ def test_multi_select_disabled_item_cannot_be_checked refute_selector "[aria-checked=true]" end + def test_multi_select_does_not_allow_server_to_check_items_on_filter_if_selections_already_made + # remote_fetch is multi-select + visit_preview(:remote_fetch) + + wait_for_items_to_load do + click_on_invoker_button + end + + # Phaser should already be selected + assert_selector "[aria-checked=true]", text: "Phaser" + + # check torpedo, uncheck phaser + click_on "Photon torpedo" + click_on "Phaser" + + wait_for_items_to_load do + filter_results(query: "ph") + end + + # server will render phaser checked, but since the user has already made selections, + # the server-rendered selections should be ignored + refute_selector "[aria-checked=true]", text: "Phaser" + assert_selector "[aria-checked=true]", text: "Photon torpedo" + end + ########## JAVASCRIPT API TESTS ############ def test_disable_item_via_js_api