From c2e54104d012208724ff8494348abc9f52f2a3f6 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 10:56:53 +1100 Subject: [PATCH 1/9] Add select/deselect all checkbox There's A Controller For That. (But I think it makes the HTML ugly..) --- app/views/admin/dfc_product_imports/index.html.haml | 9 +++++++-- config/locales/en.yml | 1 + spec/system/admin/dfc_product_import_spec.rb | 8 +++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app/views/admin/dfc_product_imports/index.html.haml b/app/views/admin/dfc_product_imports/index.html.haml index f647ab75c4..b3b38cbe69 100644 --- a/app/views/admin/dfc_product_imports/index.html.haml +++ b/app/views/admin/dfc_product_imports/index.html.haml @@ -12,13 +12,18 @@ = form.hidden_field :enterprise_id, value: @enterprise.id = form.hidden_field :catalog_json, value: @catalog_json - %table + %table{"data-controller": "checked" } + %thead + %tr + %th + %input{ type: 'checkbox', title: t('.select_all'), 'aria-label': t('.select_all'), data: { "checked-target": "all", action: "change->checked#toggleAll" } } + %th %tbody - @items.each do |supplied_product, existing_product| %tr{id: supplied_product.semanticId } %td %label - = form.check_box 'semanticIds[]', { checked: true }, supplied_product.semanticId, "" + = form.check_box 'semanticIds[]', { checked: true, data: { "action" => "change->checked#toggleCheckbox", "checked-target" => "checkbox" } }, supplied_product.semanticId, "" = supplied_product.name %td - if existing_product.present? diff --git a/config/locales/en.yml b/config/locales/en.yml index 70b26fc6e6..0a739cb660 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -851,6 +851,7 @@ en: title: "DFC product catalog" catalog_url: "%{count} products to be imported from: %{catalog_url}" enterprise: "Import to enterprise: %{enterprise_name}" + select_all: "Select/deselect all" update: Update new: New import: Import diff --git a/spec/system/admin/dfc_product_import_spec.rb b/spec/system/admin/dfc_product_import_spec.rb index 8f4d3f2487..860013ff58 100644 --- a/spec/system/admin/dfc_product_import_spec.rb +++ b/spec/system/admin/dfc_product_import_spec.rb @@ -65,7 +65,13 @@ RSpec.describe "DFC Product Import" do expect(page).to have_content "Beans - Case, 12 x 400g (can) New" expect(page).to have_content "Chia Seed, Organic - Retail pack, 300g" - uncheck "Chia Seed, Organic - Case, 8 x 300g" # don't import this one + # I can select all + uncheck "Chia Seed, Organic - Case, 8 x 300g" + check "Select/deselect all" + expect(page).to have_checked_field "Chia Seed, Organic - Case, 8 x 300g" + + # And deselect one + uncheck "Chia Seed, Organic - Case, 8 x 300g" expect { click_button "Import" From b92c0461196b34dea6df37cd0683817d1942bbb1 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 11:10:30 +1100 Subject: [PATCH 2/9] Automatically bind toggleAll action We are already specifying the element's role ('all') in the HTML. Its behaviour should be predefined; there's no need to also specify in the HTML. The eventhandler doesn't need to be cleand up on disconnect, because they are removed along with the DOM object. --- app/views/admin/dfc_product_imports/index.html.haml | 2 +- app/views/admin/order_cycles/checkout_options.html.haml | 2 +- app/views/spree/admin/orders/_table.html.haml | 2 +- app/webpacker/controllers/checked_controller.js | 2 ++ spec/javascripts/stimulus/checked_controller_test.js | 2 -- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/admin/dfc_product_imports/index.html.haml b/app/views/admin/dfc_product_imports/index.html.haml index b3b38cbe69..0bb4821cd1 100644 --- a/app/views/admin/dfc_product_imports/index.html.haml +++ b/app/views/admin/dfc_product_imports/index.html.haml @@ -16,7 +16,7 @@ %thead %tr %th - %input{ type: 'checkbox', title: t('.select_all'), 'aria-label': t('.select_all'), data: { "checked-target": "all", action: "change->checked#toggleAll" } } + %input{ type: 'checkbox', title: t('.select_all'), 'aria-label': t('.select_all'), 'data-checked-target': "all" } %th %tbody - @items.each do |supplied_product, existing_product| diff --git a/app/views/admin/order_cycles/checkout_options.html.haml b/app/views/admin/order_cycles/checkout_options.html.haml index e43d322b09..9810745e8d 100644 --- a/app/views/admin/order_cycles/checkout_options.html.haml +++ b/app/views/admin/order_cycles/checkout_options.html.haml @@ -25,7 +25,7 @@ %td.text-center - if distributor_shipping_methods.many? %label - = check_box_tag nil, nil, nil, { "data-action": "change->checked#toggleAll", "data-checked-target": "all" } + = check_box_tag nil, nil, nil, { "data-checked-target": "all" } = t(".select_all") %td %em= distributor.name diff --git a/app/views/spree/admin/orders/_table.html.haml b/app/views/spree/admin/orders/_table.html.haml index 237e309106..65ff904bec 100644 --- a/app/views/spree/admin/orders/_table.html.haml +++ b/app/views/spree/admin/orders/_table.html.haml @@ -10,7 +10,7 @@ %thead %tr %th - %input#selectAll{ type: 'checkbox', data: { "checked-target": "all", action: "change->checked#toggleAll" } } + %input#selectAll{ type: 'checkbox', 'data-checked-target': "all" } %th = t(:products_distributor) diff --git a/app/webpacker/controllers/checked_controller.js b/app/webpacker/controllers/checked_controller.js index acca8ec0dd..84589ef903 100644 --- a/app/webpacker/controllers/checked_controller.js +++ b/app/webpacker/controllers/checked_controller.js @@ -6,6 +6,8 @@ export default class extends Controller { connect() { this.toggleCheckbox(); + + this.allTarget.addEventListener("change", this.toggleAll.bind(this)); } toggleAll() { diff --git a/spec/javascripts/stimulus/checked_controller_test.js b/spec/javascripts/stimulus/checked_controller_test.js index 772d519ea0..bfa0712844 100644 --- a/spec/javascripts/stimulus/checked_controller_test.js +++ b/spec/javascripts/stimulus/checked_controller_test.js @@ -17,7 +17,6 @@ describe("CheckedController", () => { { Date: Thu, 20 Feb 2025 11:23:57 +1100 Subject: [PATCH 3/9] Automatically bind toggleCheckbox action --- app/views/admin/dfc_product_imports/index.html.haml | 2 +- app/views/admin/order_cycles/checkout_options.html.haml | 4 ++-- app/views/spree/admin/orders/_table_row.html.haml | 2 +- app/webpacker/controllers/checked_controller.js | 4 ++++ spec/javascripts/stimulus/checked_controller_test.js | 4 ---- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/views/admin/dfc_product_imports/index.html.haml b/app/views/admin/dfc_product_imports/index.html.haml index 0bb4821cd1..f4182b55fc 100644 --- a/app/views/admin/dfc_product_imports/index.html.haml +++ b/app/views/admin/dfc_product_imports/index.html.haml @@ -23,7 +23,7 @@ %tr{id: supplied_product.semanticId } %td %label - = form.check_box 'semanticIds[]', { checked: true, data: { "action" => "change->checked#toggleCheckbox", "checked-target" => "checkbox" } }, supplied_product.semanticId, "" + = form.check_box 'semanticIds[]', { checked: true, 'data-checked-target': "checkbox" }, supplied_product.semanticId, "" = supplied_product.name %td - if existing_product.present? diff --git a/app/views/admin/order_cycles/checkout_options.html.haml b/app/views/admin/order_cycles/checkout_options.html.haml index 9810745e8d..ac0fc1abf5 100644 --- a/app/views/admin/order_cycles/checkout_options.html.haml +++ b/app/views/admin/order_cycles/checkout_options.html.haml @@ -36,7 +36,7 @@ distributor_shipping_method.id, @order_cycle.distributor_shipping_methods.include?(distributor_shipping_method), id: "order_cycle_selected_distributor_shipping_method_ids_#{distributor_shipping_method.id}", - data: ({ "action" => "change->checked#toggleCheckbox", "checked-target" => "checkbox" } if distributor_shipping_method.shipping_method.frontend?) + data: ({ "checked-target" => "checkbox" } if distributor_shipping_method.shipping_method.frontend?) = distributor_shipping_method.shipping_method.name - distributor.shipping_methods.backend.each do |shipping_method| %label.disabled @@ -67,7 +67,7 @@ distributor_payment_method.id, @order_cycle.distributor_payment_methods.include?(distributor_payment_method), id: "order_cycle_selected_distributor_payment_method_ids_#{distributor_payment_method.id}", - data: ({ "action" => "change->checked#toggleCheckbox", "checked-target" => "checkbox" } if distributor_payment_method.payment_method.frontend?) + data: ({ "checked-target" => "checkbox" } if distributor_payment_method.payment_method.frontend?) = distributor_payment_method.payment_method.name - distributor.payment_methods.inactive_or_backend.each do |payment_method| %label.disabled diff --git a/app/views/spree/admin/orders/_table_row.html.haml b/app/views/spree/admin/orders/_table_row.html.haml index 7cc4aae10f..4027bcf6da 100644 --- a/app/views/spree/admin/orders/_table_row.html.haml +++ b/app/views/spree/admin/orders/_table_row.html.haml @@ -1,6 +1,6 @@ %tr{ id: dom_id(order), class: "state-#{order.state}" } %td.align-left - %input{type: 'checkbox', value: order.id, name: 'bulk_ids[]', "data-checked-target": "checkbox", "data-action": "change->checked#toggleCheckbox" } + %input{type: 'checkbox', value: order.id, name: 'bulk_ids[]', "data-checked-target": "checkbox" } %td.align-left = order.distributor.name %td.align-left diff --git a/app/webpacker/controllers/checked_controller.js b/app/webpacker/controllers/checked_controller.js index 84589ef903..fc7ac72b18 100644 --- a/app/webpacker/controllers/checked_controller.js +++ b/app/webpacker/controllers/checked_controller.js @@ -8,6 +8,10 @@ export default class extends Controller { this.toggleCheckbox(); this.allTarget.addEventListener("change", this.toggleAll.bind(this)); + + this.checkboxTargets.forEach((checkbox) => { + checkbox.addEventListener("change", this.toggleCheckbox.bind(this)); + }); } toggleAll() { diff --git a/spec/javascripts/stimulus/checked_controller_test.js b/spec/javascripts/stimulus/checked_controller_test.js index bfa0712844..1018f1ff6d 100644 --- a/spec/javascripts/stimulus/checked_controller_test.js +++ b/spec/javascripts/stimulus/checked_controller_test.js @@ -21,12 +21,10 @@ describe("CheckedController", () => { `; @@ -90,13 +88,11 @@ describe("CheckedController", () => { From 57fb85514773575f28e2e0aec3c5195a9cdcf2b2 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 11:43:09 +1100 Subject: [PATCH 4/9] Ensure label reaches to edge of table cell Best viewed with whitespace ignored --- .../admin/dfc_product_imports/index.html.haml | 63 ++++++++++--------- app/webpacker/css/admin_v3/all.scss | 1 + .../admin_v3/pages/dfc_product_imports.scss | 11 ++++ 3 files changed, 44 insertions(+), 31 deletions(-) create mode 100644 app/webpacker/css/admin_v3/pages/dfc_product_imports.scss diff --git a/app/views/admin/dfc_product_imports/index.html.haml b/app/views/admin/dfc_product_imports/index.html.haml index f4182b55fc..80f780cff2 100644 --- a/app/views/admin/dfc_product_imports/index.html.haml +++ b/app/views/admin/dfc_product_imports/index.html.haml @@ -1,36 +1,37 @@ - content_for :page_title do #{t(".title")} -= render partial: 'spree/admin/shared/product_sub_menu' - -%p= t('.catalog_url', count: @items.count, catalog_url: @catalog_url) -%p= t('.enterprise', enterprise_name: @enterprise.name) -%br - -= form_with url: main_app.import_admin_dfc_product_imports_path do |form| - -# This is a very inefficient way of holding a json blob. Maybe base64 encode or store as a temporary file - = form.hidden_field :enterprise_id, value: @enterprise.id - = form.hidden_field :catalog_json, value: @catalog_json - - %table{"data-controller": "checked" } - %thead - %tr - %th - %input{ type: 'checkbox', title: t('.select_all'), 'aria-label': t('.select_all'), 'data-checked-target': "all" } - %th - %tbody - - @items.each do |supplied_product, existing_product| - %tr{id: supplied_product.semanticId } - %td - %label - = form.check_box 'semanticIds[]', { checked: true, 'data-checked-target': "checkbox" }, supplied_product.semanticId, "" - = supplied_product.name - %td - - if existing_product.present? - = t(".update") - = link_to(existing_product.id, edit_admin_product_path(existing_product)) - - else - = t(".new") +#dfc_product_imports + = render partial: 'spree/admin/shared/product_sub_menu' + %p= t('.catalog_url', count: @items.count, catalog_url: @catalog_url) + %p= t('.enterprise', enterprise_name: @enterprise.name) %br - = form.submit t(".import") + + = form_with url: main_app.import_admin_dfc_product_imports_path do |form| + -# This is a very inefficient way of holding a json blob. Maybe base64 encode or store as a temporary file + = form.hidden_field :enterprise_id, value: @enterprise.id + = form.hidden_field :catalog_json, value: @catalog_json + + %table{"data-controller": "checked" } + %thead + %tr + %th + %input{ type: 'checkbox', title: t('.select_all'), 'aria-label': t('.select_all'), 'data-checked-target': "all" } + %th + %tbody + - @items.each do |supplied_product, existing_product| + %tr{id: supplied_product.semanticId } + %td + %label + = form.check_box 'semanticIds[]', { checked: true, 'data-checked-target': "checkbox" }, supplied_product.semanticId, "" + = supplied_product.name + %td + - if existing_product.present? + = t(".update") + = link_to(existing_product.id, edit_admin_product_path(existing_product)) + - else + = t(".new") + + %br + = form.submit t(".import") diff --git a/app/webpacker/css/admin_v3/all.scss b/app/webpacker/css/admin_v3/all.scss index 32eba7ddde..a339a20457 100644 --- a/app/webpacker/css/admin_v3/all.scss +++ b/app/webpacker/css/admin_v3/all.scss @@ -133,3 +133,4 @@ @import "terms_of_service_banner"; // admin_v3 @import "pages/product_preview"; // admin_v3 +@import "pages/dfc_product_imports"; // admin_v3 diff --git a/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss b/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss new file mode 100644 index 0000000000..dabcb805d7 --- /dev/null +++ b/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss @@ -0,0 +1,11 @@ + +#dfc_product_imports { + // Ensure label reaches to edge of table cell + td:has(> label) { + padding: 0; + } + td > label { + display: block; + padding: 7px 5px; + } +} \ No newline at end of file From 6e7766a2c2c82ef13e74fd90178a475c585dba6e Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 11:44:18 +1100 Subject: [PATCH 5/9] Increase font size --- app/webpacker/css/admin_v3/pages/dfc_product_imports.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss b/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss index dabcb805d7..4f96a6b3c3 100644 --- a/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss +++ b/app/webpacker/css/admin_v3/pages/dfc_product_imports.scss @@ -7,5 +7,6 @@ td > label { display: block; padding: 7px 5px; + font-size: inherit; } } \ No newline at end of file From 4cb1b0a48ecb997e86934330fa00770ca5e4ce66 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 12:09:41 +1100 Subject: [PATCH 6/9] Add total count of selected items I noticed there's a controller for that too, so might as well make use of it. The orders page has it at the top, because that's where the bulk action menu is. But on this page, the action is the import button so I put it there. --- app/views/admin/dfc_product_imports/index.html.haml | 7 +++++-- config/locales/en.yml | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/views/admin/dfc_product_imports/index.html.haml b/app/views/admin/dfc_product_imports/index.html.haml index 80f780cff2..720705d373 100644 --- a/app/views/admin/dfc_product_imports/index.html.haml +++ b/app/views/admin/dfc_product_imports/index.html.haml @@ -8,12 +8,12 @@ %p= t('.enterprise', enterprise_name: @enterprise.name) %br - = form_with url: main_app.import_admin_dfc_product_imports_path do |form| + = form_with url: main_app.import_admin_dfc_product_imports_path, html: { "data-controller": "checked" } do |form| -# This is a very inefficient way of holding a json blob. Maybe base64 encode or store as a temporary file = form.hidden_field :enterprise_id, value: @enterprise.id = form.hidden_field :catalog_json, value: @catalog_json - %table{"data-controller": "checked" } + %table %thead %tr %th @@ -33,5 +33,8 @@ - else = t(".new") + %span{ "data-controller": "checked-feedback", "data-checked-feedback-translation-value": "admin.dfc_product_imports.index.selected" } + = t(".selected", count: @items.count) + %br = form.submit t(".import") diff --git a/config/locales/en.yml b/config/locales/en.yml index 0a739cb660..f43f8667b2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -854,6 +854,10 @@ en: select_all: "Select/deselect all" update: Update new: New + selected: + zero: "0 selected" + one: "1 selected" + other: "%{count} selected" import: Import import: title: "DFC product catalog import" From e31c16df43e853e2a29b28ee0b3908684d2f6769 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 12:46:23 +1100 Subject: [PATCH 7/9] Delegate events to the parent element Ok so I wasn't as smart as I thought I was. The stimulus controller knows when its element is added/removed from the DOM (and calls connect/disconnect appropriately). But if any child elements are added, they don't automatically have my new event handlers. So I borrowed jQuery's event delegation concept, and listen for any events that 'bubble' up to the controller element, and delegate them as needed. Alternatively, maybe I could have used a Mutation Observer, but I think it's best to avoid where possible. Or of course, we could just revert my change and keep the 'data-action's in the HTML. I'm curious to hear opinions on this.." --- app/webpacker/controllers/checked_controller.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/app/webpacker/controllers/checked_controller.js b/app/webpacker/controllers/checked_controller.js index fc7ac72b18..e29a4391e6 100644 --- a/app/webpacker/controllers/checked_controller.js +++ b/app/webpacker/controllers/checked_controller.js @@ -7,11 +7,7 @@ export default class extends Controller { connect() { this.toggleCheckbox(); - this.allTarget.addEventListener("change", this.toggleAll.bind(this)); - - this.checkboxTargets.forEach((checkbox) => { - checkbox.addEventListener("change", this.toggleCheckbox.bind(this)); - }); + this.element.addEventListener("change", this.#toggleChangeListener.bind(this), {passive: true}); } toggleAll() { @@ -39,6 +35,15 @@ export default class extends Controller { // private + // Delegate events for targets (this ensures we catch events from newly-added elements after an ajax action) + #toggleChangeListener(event) { + if (event.target == this.allTarget) { + this.toggleAll(); + } else if (this.checkboxTargets.includes(event.target)) { + this.toggleCheckbox(); + } + } + #checkedCount() { return this.checkboxTargets.filter((checkbox) => checkbox.checked).length; } From 7acc78b6d80bb06972a37f2ad546320b32c5ec3b Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 12:49:25 +1100 Subject: [PATCH 8/9] Revert "Delegate events to the parent element" This reverts commit e31c16df43e853e2a29b28ee0b3908684d2f6769. --- app/webpacker/controllers/checked_controller.js | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/app/webpacker/controllers/checked_controller.js b/app/webpacker/controllers/checked_controller.js index e29a4391e6..fc7ac72b18 100644 --- a/app/webpacker/controllers/checked_controller.js +++ b/app/webpacker/controllers/checked_controller.js @@ -7,7 +7,11 @@ export default class extends Controller { connect() { this.toggleCheckbox(); - this.element.addEventListener("change", this.#toggleChangeListener.bind(this), {passive: true}); + this.allTarget.addEventListener("change", this.toggleAll.bind(this)); + + this.checkboxTargets.forEach((checkbox) => { + checkbox.addEventListener("change", this.toggleCheckbox.bind(this)); + }); } toggleAll() { @@ -35,15 +39,6 @@ export default class extends Controller { // private - // Delegate events for targets (this ensures we catch events from newly-added elements after an ajax action) - #toggleChangeListener(event) { - if (event.target == this.allTarget) { - this.toggleAll(); - } else if (this.checkboxTargets.includes(event.target)) { - this.toggleCheckbox(); - } - } - #checkedCount() { return this.checkboxTargets.filter((checkbox) => checkbox.checked).length; } From 7444ddccd1dd8340bb18ac0650bb4259c0ff5f99 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Feb 2025 12:52:27 +1100 Subject: [PATCH 9/9] Wait, There's A Lifecycle Callback For That --- app/webpacker/controllers/checked_controller.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/webpacker/controllers/checked_controller.js b/app/webpacker/controllers/checked_controller.js index fc7ac72b18..cc0c8392bd 100644 --- a/app/webpacker/controllers/checked_controller.js +++ b/app/webpacker/controllers/checked_controller.js @@ -6,12 +6,14 @@ export default class extends Controller { connect() { this.toggleCheckbox(); + } - this.allTarget.addEventListener("change", this.toggleAll.bind(this)); + allTargetConnected(allTarget) { + allTarget.addEventListener("change", this.toggleAll.bind(this)); + } - this.checkboxTargets.forEach((checkbox) => { - checkbox.addEventListener("change", this.toggleCheckbox.bind(this)); - }); + checkboxTargetConnected(checkboxTarget) { + checkboxTarget.addEventListener("change", this.toggleCheckbox.bind(this)); } toggleAll() {