From 884d6f15ff9893228d8c7daa166a22e8f015882b Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Sun, 25 Feb 2024 16:27:40 +0100 Subject: [PATCH 1/5] Replace a divs controller by an html details one - checked_controller close details element on checkboxes - dropdown_controller.js is to rebuild controller from many divs to be hidden and visible to an html detail elmnt one - details html element styling --- .../controllers/checked_controller.js | 8 ++++ .../controllers/dropdown_controller.js | 41 ++++++------------- app/webpacker/css/admin/dropdown.scss | 20 +++++++++ .../css/admin_v3/components/dropdown.scss | 20 +++++++++ 4 files changed, 61 insertions(+), 28 deletions(-) diff --git a/app/webpacker/controllers/checked_controller.js b/app/webpacker/controllers/checked_controller.js index 6317635fa4..acca8ec0dd 100644 --- a/app/webpacker/controllers/checked_controller.js +++ b/app/webpacker/controllers/checked_controller.js @@ -41,6 +41,13 @@ export default class extends Controller { return this.countValue === this.checkboxTargets.length; } + #closeDetails(elmnt) { + if (elmnt.getElementsByTagName('details').length == 0) + return; + + Array.from(elmnt.getElementsByTagName('details')).forEach((element) => element.open = false); + } + #toggleDisabled() { if (!this.hasDisableTarget) { return; @@ -48,6 +55,7 @@ export default class extends Controller { if (this.#checkedCount() === 0) { this.disableTargets.forEach((element) => element.classList.add("disabled")); + this.disableTargets.forEach(this.#closeDetails); } else { this.disableTargets.forEach((element) => element.classList.remove("disabled")); } diff --git a/app/webpacker/controllers/dropdown_controller.js b/app/webpacker/controllers/dropdown_controller.js index 19a3dc8b7a..844289e12c 100644 --- a/app/webpacker/controllers/dropdown_controller.js +++ b/app/webpacker/controllers/dropdown_controller.js @@ -1,44 +1,29 @@ import { Controller } from "stimulus"; export default class extends Controller { - static targets = ["arrow", "menu"]; connect() { - this.collapsedClasses = this.arrowTarget.dataset.collapsedClass.split(" "); - this.expandedClasses = this.arrowTarget.dataset.expandedClass.split(" "); - this.#hide(); - document.addEventListener("click", this.#onBodyClick.bind(this)); + document.body.addEventListener("click", this.#close.bind(this)); + this.element.addEventListener("click", this.#stopPropagation.bind(this)); } disconnect() { - document.removeEventListener("click", this.#onBodyClick); + document.removeEventListener("click", this.#close); + document.removeEventListener("click", this.#stopPropagation); } - toggle() { - if (this.element.classList.contains("disabled")) { - return; - } - if (this.menuTarget.classList.contains("hidden")) { - this.#show(); - } else { - this.#hide(); - } + closeOnMenu(event) { + this.#close(); + this.#stopPropagation(event); } - #onBodyClick(event) { - if (!this.element.contains(event.target)) { - this.#hide(); - } + // private + + #close(event) { + this.element.open = false; } - #show() { - this.menuTarget.classList.remove("hidden"); - this.arrowTarget.classList.remove(...this.collapsedClasses); - this.arrowTarget.classList.add(...this.expandedClasses); - } - #hide() { - this.menuTarget.classList.add("hidden"); - this.arrowTarget.classList.remove(...this.expandedClasses); - this.arrowTarget.classList.add(...this.collapsedClasses); + #stopPropagation(event) { + event.stopPropagation(); } } diff --git a/app/webpacker/css/admin/dropdown.scss b/app/webpacker/css/admin/dropdown.scss index 64c98e7ddf..443d8b46e3 100644 --- a/app/webpacker/css/admin/dropdown.scss +++ b/app/webpacker/css/admin/dropdown.scss @@ -47,6 +47,7 @@ &.disabled { opacity: 0.5; + pointer-events: none; &:hover { cursor: default; @@ -179,6 +180,25 @@ background-color: #ededed; } } + + > details > summary { + display: inline-block; + list-style: none; + width: auto; + text-transform: uppercase; + font-size: 85%; + font-weight: 600; + } + + > details > summary:after { + content: "\f0d7"; + font-family: FontAwesome; + } + + > details[open] > summary:after { + content: "\f0d8"; + font-family: FontAwesome; + } } .ofn-drop-down-v2 { diff --git a/app/webpacker/css/admin_v3/components/dropdown.scss b/app/webpacker/css/admin_v3/components/dropdown.scss index 6a93884aeb..bf2a36c75b 100644 --- a/app/webpacker/css/admin_v3/components/dropdown.scss +++ b/app/webpacker/css/admin_v3/components/dropdown.scss @@ -47,6 +47,7 @@ &.disabled { opacity: 0.5; + pointer-events: none; &:hover { cursor: default; @@ -179,6 +180,25 @@ background-color: #ededed; } } + + > details > summary { + display: inline-block; + list-style: none; + width: auto; + text-transform: uppercase; + font-size: 85%; + font-weight: 600; + } + + > details > summary:after { + content: "\f0d7"; + font-family: FontAwesome; + } + + > details[open] > summary:after { + content: "\f0d8"; + font-family: FontAwesome; + } } .ofn-drop-down-v2 { From 428b9b273c6148bd23a82e99caba351292576449 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Sun, 25 Feb 2024 16:33:29 +0100 Subject: [PATCH 2/5] Replace old dropdown controller by new one - menu items line are unchanged only beggining of file modified --- .../admin/orders/_bulk_actions.html.haml | 35 +++++++++---------- .../spree/admin/shared/_order_links.html.haml | 35 ++++++++++--------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/app/views/spree/admin/orders/_bulk_actions.html.haml b/app/views/spree/admin/orders/_bulk_actions.html.haml index 7616334c1f..74c032ed55 100644 --- a/app/views/spree/admin/orders/_bulk_actions.html.haml +++ b/app/views/spree/admin/orders/_bulk_actions.html.haml @@ -3,23 +3,22 @@ %span{ "data-controller": "checked-feedback", "data-checked-feedback-translation-value": "spree.admin.orders.index.selected" } = t("spree.admin.orders.index.selected", count: 0) - %div.plain.ofn-drop-down.disabled{ "data-checked-target": "disable", "data-controller": "dropdown", "data-action": "click->dropdown#toggle" } - %span{ class: 'icon-reorder' } - ="#{t('admin.actions')}".html_safe - %span - %i{ "data-dropdown-target": "arrow", "data-expanded-class": "icon-caret-up", "data-collapsed-class": "icon-caret-down" } - - %div.menu{ "data-dropdown-target": "menu" } - %div.menu_item - %span.name{ "data-controller": "modal-link", "data-action": "click->modal-link#open", "data-modal-link-target-value": "resend_confirmation" } - = t('spree.admin.orders.index.resend_confirmation') - - if Spree::Config[:enable_invoices?] + %div.plain.ofn-drop-down.disabled{ "data-checked-target": "disable" } + %details{"data-controller": "dropdown"} + %summary + %span.icon-reorder + ="#{t('admin.actions')}".html_safe + %div.menu{"data-action": "click->dropdown#closeOnMenu"} %div.menu_item - %span.name{ "data-controller": "modal-link", "data-action": "click->modal-link#open", "data-modal-link-target-value": "send_invoice" } - = t('spree.admin.orders.index.send_invoice') + %span.name{ "data-controller": "modal-link", "data-action": "click->modal-link#open", "data-modal-link-target-value": "resend_confirmation" } + = t('spree.admin.orders.index.resend_confirmation') + - if Spree::Config[:enable_invoices?] + %div.menu_item + %span.name{ "data-controller": "modal-link", "data-action": "click->modal-link#open", "data-modal-link-target-value": "send_invoice" } + = t('spree.admin.orders.index.send_invoice') + %div.menu_item + %span.name{ "data-controller": "bulk-actions", "data-action": "click->bulk-actions#perform", "data-bulk-actions-reflex-value": "Admin::Orders#bulk_invoice" } + = t('spree.admin.orders.index.print_invoices') %div.menu_item - %span.name{ "data-controller": "bulk-actions", "data-action": "click->bulk-actions#perform", "data-bulk-actions-reflex-value": "Admin::Orders#bulk_invoice" } - = t('spree.admin.orders.index.print_invoices') - %div.menu_item - %span.name{ "data-controller": "modal-link", "data-action": "click->modal-link#open", "data-modal-link-target-value": "cancel_orders" } - = t('spree.admin.orders.index.cancel_orders') + %span.name{ "data-controller": "modal-link", "data-action": "click->modal-link#open", "data-modal-link-target-value": "cancel_orders" } + = t('spree.admin.orders.index.cancel_orders') diff --git a/app/views/spree/admin/shared/_order_links.html.haml b/app/views/spree/admin/shared/_order_links.html.haml index 41508a2ad2..9af75fffd8 100644 --- a/app/views/spree/admin/shared/_order_links.html.haml +++ b/app/views/spree/admin/shared/_order_links.html.haml @@ -1,20 +1,21 @@ %li.links-dropdown#links-dropdown - .ofn-drop-down{"data-controller": "dropdown", "data-action": "click->dropdown#toggle" } - %span - %i.icon-check - = I18n.t 'admin.actions' - %i{ "data-dropdown-target": "arrow", "data-expanded-class": "icon-caret-up", "data-collapsed-class": "icon-caret-down" } - %div.menu.hidden{"data-dropdown-target": "menu"} - - order_links(@order).each do |link| - - if link[:name] == t(:ship_order) - %a.menu_item{ href: link[:url], target: link[:target] || "_self", data: { "modal-link-target-value": dom_id(@order, :ship), "action": "click->modal-link#open", "controller": "modal-link" } } - %span - %i{ class: link[:icon] } - %span=link[:name] - - else - %a.menu_item{ href: link[:url], target: link[:target] || "_self", data: { method: link[:method], "ujs-navigate": link[:method] ? "false" : "undefined", confirm: link[:confirm] } } - %span - %i{ class: link[:icon] } - %span=link[:name] + .ofn-drop-down + %details{"data-controller": "dropdown"} + %summary + %span + %i.icon-check + = I18n.t 'admin.actions' + %div.menu{"data-action": "click->dropdown#closeOnMenu"} + - order_links(@order).each do |link| + - if link[:name] == t(:ship_order) + %a.menu_item{ href: link[:url], target: link[:target] || "_self", data: { "modal-link-target-value": dom_id(@order, :ship), "action": "click->modal-link#open", "controller": "modal-link" } } + %span + %i{ class: link[:icon] } + %span=link[:name] + - else + %a.menu_item{ href: link[:url], target: link[:target] || "_self", data: { method: link[:method], "ujs-navigate": link[:method] ? "false" : "undefined", confirm: link[:confirm] } } + %span + %i{ class: link[:icon] } + %span=link[:name] = render 'spree/admin/shared/custom-confirm' From b08623df232e54024985d4178bb671142413540d Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Sun, 25 Feb 2024 16:40:44 +0100 Subject: [PATCH 3/5] Sytem specs + controller spec - controller spec is lighter since it is based on an html element --- .../stimulus/dropdown_controller_test.js | 62 ++++++------------- spec/system/admin/order_spec.rb | 4 +- spec/system/admin/orders_spec.rb | 5 +- 3 files changed, 23 insertions(+), 48 deletions(-) diff --git a/spec/javascripts/stimulus/dropdown_controller_test.js b/spec/javascripts/stimulus/dropdown_controller_test.js index 2f1bceaf4d..9e8c60c5ed 100644 --- a/spec/javascripts/stimulus/dropdown_controller_test.js +++ b/spec/javascripts/stimulus/dropdown_controller_test.js @@ -13,13 +13,20 @@ describe("Dropdown controller", () => { describe("Controller", () => { beforeEach(() => { - document.body.innerHTML = `
- - - - + document.body.innerHTML = `
+
`; }); @@ -27,48 +34,15 @@ describe("Dropdown controller", () => { document.body.innerHTML = ""; }); - it("hide menu by default", () => { - const menu = document.getElementById("menu"); - expect(menu.classList.contains("hidden")).toBe(true); - }); - - it("show menu when toggle and add/remove class on arrow", () => { - const dropdown = document.getElementById("dropdown"); - const arrow = document.getElementById("arrow"); - const menu = document.getElementById("menu"); - expect(menu.classList.contains("hidden")).toBe(true); - expect(arrow.classList.contains("expandedClass")).toBe(false); - expect(arrow.classList.contains("expandedClass2")).toBe(false); - expect(arrow.classList.contains("collapsedClass")).toBe(true); - - dropdown.click(); - - expect(menu.classList.contains("hidden")).toBe(false); - expect(arrow.classList.contains("expandedClass")).toBe(true); - expect(arrow.classList.contains("expandedCLass2")).toBe(true); - expect(arrow.classList.contains("collapsedClass")).toBe(false); - }); - it ("hide menu when click outside", () => { const dropdown = document.getElementById("dropdown"); const menu = document.getElementById("menu"); - dropdown.click(); - expect(menu.classList.contains("hidden")).toBe(false); - + //open the details + dropdown.toggleAttribute('open') + //click elsewhere document.body.click(); - expect(menu.classList.contains("hidden")).toBe(true); - }); - - it ("do not display menu when disabled", () => { - const dropdown = document.getElementById("dropdown"); - const container = document.getElementById("container"); - const menu = document.getElementById("menu"); - container.classList.add("disabled"); - - dropdown.click(); - - expect(menu.classList.contains("hidden")).toBe(true); + expect(dropdown.open).toBe(false); }); }); }); diff --git a/spec/system/admin/order_spec.rb b/spec/system/admin/order_spec.rb index 8ec2dc5867..b4de995c69 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -719,7 +719,7 @@ describe ' it "should not display links but a js alert" do visit spree.edit_admin_order_path(order) - find("#links-dropdown .ofn-drop-down").click + find("#links-dropdown .ofn-drop-down details").click expect(page).to have_link "Send Invoice", href: "#" expect(page).to have_link "Print Invoice", href: "#" @@ -729,7 +729,7 @@ describe ' expect(message) .to eq "#{distributor1.name} must have a valid ABN before invoices can be used." - find("#links-dropdown .ofn-drop-down").click + find("#links-dropdown .ofn-drop-down details").click message = accept_prompt do click_link "Send Invoice" end diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index b9f29d296b..22bd0b7841 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -497,8 +497,9 @@ describe ' expect(page.find( "#listing_orders tbody tr td:first-child input[type=checkbox]" )).to_not be_checked - # disables print invoices button - page.find("span.icon-reorder", text: "ACTIONS").click + # disables print invoices button not clickable + expect { find("span.icon-reorder", text: "ACTIONS").click } + .to raise_error(Capybara::Cuprite::MouseEventFailed) expect(page).to_not have_content "Print Invoices" end end From f62b32a3b986480be22c4897c85d065f1e41f6a3 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 28 Feb 2024 08:15:34 +0100 Subject: [PATCH 4/5] Requested changes after review - modified css to increase clicking area - modified spec to more straightfoward and more user oriented link --- app/webpacker/css/admin/dropdown.scss | 7 +++++++ app/webpacker/css/admin_v3/components/dropdown.scss | 7 +++++++ spec/system/admin/order_spec.rb | 4 ++-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/webpacker/css/admin/dropdown.scss b/app/webpacker/css/admin/dropdown.scss index 443d8b46e3..c18e90fea0 100644 --- a/app/webpacker/css/admin/dropdown.scss +++ b/app/webpacker/css/admin/dropdown.scss @@ -181,6 +181,11 @@ } } + > details { + margin: -7px -15px; + padding: 7px 15px; + } + > details > summary { display: inline-block; list-style: none; @@ -188,6 +193,8 @@ text-transform: uppercase; font-size: 85%; font-weight: 600; + margin: -8px -15px; + padding: 8px 15px; } > details > summary:after { diff --git a/app/webpacker/css/admin_v3/components/dropdown.scss b/app/webpacker/css/admin_v3/components/dropdown.scss index bf2a36c75b..8a101d1c8d 100644 --- a/app/webpacker/css/admin_v3/components/dropdown.scss +++ b/app/webpacker/css/admin_v3/components/dropdown.scss @@ -181,6 +181,11 @@ } } + > details { + margin: -7px -15px; + padding: 7px 15px; + } + > details > summary { display: inline-block; list-style: none; @@ -188,6 +193,8 @@ text-transform: uppercase; font-size: 85%; font-weight: 600; + margin: -8px -15px; + padding: 8px 15px; } > details > summary:after { diff --git a/spec/system/admin/order_spec.rb b/spec/system/admin/order_spec.rb index b4de995c69..3d6c083d00 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -719,7 +719,7 @@ describe ' it "should not display links but a js alert" do visit spree.edit_admin_order_path(order) - find("#links-dropdown .ofn-drop-down details").click + find("summary", text: "ACTIONS").click expect(page).to have_link "Send Invoice", href: "#" expect(page).to have_link "Print Invoice", href: "#" @@ -729,7 +729,7 @@ describe ' expect(message) .to eq "#{distributor1.name} must have a valid ABN before invoices can be used." - find("#links-dropdown .ofn-drop-down details").click + find("summary", text: "ACTIONS").click message = accept_prompt do click_link "Send Invoice" end From 5cfac3d2c7a08a855735a8b731c85f6b6d4b65e0 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 29 Feb 2024 10:10:57 +1100 Subject: [PATCH 5/5] Add comments --- app/webpacker/css/admin/dropdown.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/webpacker/css/admin/dropdown.scss b/app/webpacker/css/admin/dropdown.scss index c18e90fea0..ab3a61d5bb 100644 --- a/app/webpacker/css/admin/dropdown.scss +++ b/app/webpacker/css/admin/dropdown.scss @@ -182,6 +182,7 @@ } > details { + // Override padding on ofn-drop-down-style margin: -7px -15px; padding: 7px 15px; } @@ -193,6 +194,7 @@ text-transform: uppercase; font-size: 85%; font-weight: 600; + // Override padding on ofn-drop-down-style to increase clickable area margin: -8px -15px; padding: 8px 15px; }