From 72ce3a01a9619878e2e7bec60be85e0d5d63a48a Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 11 Apr 2024 10:00:40 +1000 Subject: [PATCH 01/13] Ensure search terms and filters are retained when saving --- .../admin/products_v3_controller.rb | 4 ++- app/views/admin/products_v3/_table.html.haml | 5 +++- .../system/admin/products_v3/products_spec.rb | 25 +++++++++++++++---- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index fb5efe5d1a..819031652a 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -18,7 +18,9 @@ module Admin if product_set.save flash[:success] = I18n.t('admin.products_v3.bulk_update.success') - redirect_to [:index, { page: @page, per_page: @per_page }] + redirect_to [:index, + { page: @page, per_page: @per_page, search_term: @search_term, + producer_id: @producer_id, category_id: @category_id }] elsif product_set.errors.present? @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count } diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 4a34b81258..1cf0f78e2d 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -7,6 +7,9 @@ = hidden_field_tag :page, @page = hidden_field_tag :per_page, @per_page + = hidden_field_tag :search_term, @search_term + = hidden_field_tag :producer_id, @producer_id + = hidden_field_tag :category_id, @category_id %table.products %colgroup @@ -39,7 +42,7 @@ -# Y products could not be saved correctly. Please review errors and try again = t('.error_summary.invalid', count: @error_counts[:invalid]) .form-buttons - %a.button.reset.medium{ href: admin_products_path(page: @page, per_page: @per_page) } + %a.button.reset.medium{ href: admin_products_path(page: @page, per_page: @per_page, search_term: @search_term, producer_id: @producer_id, category_id: @category_id) } = t('.reset') = form.submit t('.save'), class: "medium" %tr diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 74dcc2d051..2afde20c1f 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -130,9 +130,9 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do # create a product with a different supplier let!(:producer) { create(:supplier_enterprise, name: "Producer 1") } - let!(:product_by_supplier) { create(:simple_product, supplier: producer) } + let!(:product_by_supplier) { create(:simple_product, name: "Apples", supplier: producer) } - it "can search for a product" do + it "can search for and update a product" do visit admin_products_url search_by_producer "Producer 1" @@ -140,6 +140,22 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do # expect(page).to have_content "1 product found for your search criteria." expect(page).to have_select "producer_id", selected: "Producer 1" expect_products_count_to_be 1 + + within row_containing_name("Apples") do + fill_in "Name", with: "Pommes" + end + + expect { + click_button "Save changes" + + expect(page).to have_content "Changes saved" + product_by_supplier.reload + }.to change { product_by_supplier.name }.to("Pommes") + + # Search is still applied + # expect(page).to have_content "1 product found for your search criteria." + expect(page).to have_select "producer_id", selected: "Producer 1" + expect_products_count_to_be 1 end end @@ -976,13 +992,12 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do end def search_by_producer(producer) - # TODO: use a helper to more reliably select the tom-select component - select producer, from: "producer_id" + tomselect_select producer, from: "producer_id" click_button "Search" end def search_by_category(category) - select category, from: "category_id" + tomselect_select category, from: "category_id" click_button "Search" end From f17b0d176bb82c18f9502f6f0dd06fdaac73a265 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 11 Apr 2024 13:42:36 +1000 Subject: [PATCH 02/13] Enable Turbo Drive on products page Forms now load without a full page rebuild. This is not really faster, but a bit smoother because it avoids a full page render in the browser. The default Turbo loading indicator is shown (blue line at top). But the bulk_update form breaks... hmm On to the next level! --- app/controllers/admin/products_v3_controller.rb | 2 +- app/views/admin/products_v3/_table.html.haml | 2 +- app/views/admin/products_v3/index.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 819031652a..667706aeba 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -24,7 +24,7 @@ module Admin elsif product_set.errors.present? @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count } - render "index", locals: { producers:, categories:, flash: } + render "index", status: :unprocessable_entity, locals: { producers:, categories:, flash: } end end diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 1cf0f78e2d..11f515ee28 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,6 +1,6 @@ = form_with url: bulk_update_admin_products_path, method: :post, id: "products-form", builder: BulkFormBuilder, - html: { data: { controller: "bulk-form", 'bulk-form-disable-selector-value': "#sort,#filters", + html: { data: { remote: true, controller: "bulk-form", 'bulk-form-disable-selector-value': "#sort,#filters", 'bulk-form-error-value': defined?(@error_counts), } } do |form| = render(partial: "admin/shared/flashes", locals: { flashes: }) if defined? flashes diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index 2ac344c476..4d6a384c01 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -13,7 +13,7 @@ = render partial: 'spree/admin/shared/product_sub_menu' -#products_v3_page{ "data-controller": "products" } +#products_v3_page{ "data-controller": "products", 'data-turbo': true } .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } .spinner-container .spinner From 9d6ef2f730c6a501e8aa2694bff3adae5fcc0667 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 11 Apr 2024 16:50:20 +1000 Subject: [PATCH 03/13] Avoid style issues with Turbo But the filter dropdowns still get duplicated. So weird.. --- app/views/admin/products_v3/index.html.haml | 2 -- app/webpacker/css/admin/products_v3.scss | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index 4d6a384c01..dd3034ca42 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -1,5 +1,3 @@ -- content_for :body_class do - products_v3_page - content_for :page_title do = t('.header.title') - content_for :page_actions do diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index a2c835ccd0..8cb8ccbce2 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -1,5 +1,6 @@ // Customisations for the new Bulk Edit Products page only -.products_v3_page { +// Scoped to containing div, because Turbo messes with body classes +#products_v3_page { #content > .row:first-child > .container:first-child { // Allow table to extend to full width of available screen space // TODO: move this to a generic rule, eg body.full-width{}. Then it can be included on any page. From 6c9a47854a205f41c21fd8f5f0c51450535a2a75 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 10 Apr 2024 17:22:49 +1000 Subject: [PATCH 04/13] Submit forms with Turbo Frame Now the filters, pagination and product forms submit and load within the frame, and work perfectly, yay! It's still building the whole page on the server.. I think we need Turbo Streams if we want to send back just a partial. --- app/views/admin/products_v3/_content.html.haml | 2 +- app/views/admin/products_v3/_filters.html.haml | 2 +- app/views/admin/products_v3/_table.html.haml | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index 64e6620ef9..3f8f206cbc 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -1,4 +1,4 @@ -#products-content +%turbo-frame#products-content .container .sixteen.columns = render partial: 'admin/shared/flashes', locals: { flashes: } if defined? flashes diff --git a/app/views/admin/products_v3/_filters.html.haml b/app/views/admin/products_v3/_filters.html.haml index 30a95abc08..469ec1bf57 100644 --- a/app/views/admin/products_v3/_filters.html.haml +++ b/app/views/admin/products_v3/_filters.html.haml @@ -1,4 +1,4 @@ -= form_with url: admin_products_path, id: "filters", method: :get, data: { remote: false, "search-target": "form" } do += form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form" } do = hidden_field_tag :page, nil, class: "page" = hidden_field_tag :per_page, nil, class: "per-page" diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 11f515ee28..e14925b539 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,6 +1,7 @@ = form_with url: bulk_update_admin_products_path, method: :post, id: "products-form", builder: BulkFormBuilder, - html: { data: { remote: true, controller: "bulk-form", 'bulk-form-disable-selector-value': "#sort,#filters", + html: { data: { controller: "bulk-form", + 'bulk-form-disable-selector-value': "#sort,#filters", 'bulk-form-error-value': defined?(@error_counts), } } do |form| = render(partial: "admin/shared/flashes", locals: { flashes: }) if defined? flashes From 508ebab75b239c6cd5734245fcccab41bd208c05 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 11 Apr 2024 15:19:28 +1000 Subject: [PATCH 05/13] Add loading spinner to turbo frame That was surprisingly easy. Note that it's still shared with SR. It hides a bit early though: when the web response returns, but before the DOM has been rendered. Something to optimise in the future. --- app/views/admin/products_v3/_content.html.haml | 4 ++++ app/views/admin/products_v3/index.html.haml | 5 ----- app/webpacker/css/admin_v3/components/spinner.scss | 9 ++++++++- config/locales/en.yml | 1 + 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index 3f8f206cbc..40e22e4bc5 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -1,4 +1,8 @@ %turbo-frame#products-content + .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } + .spinner-container + .spinner + = t('.loading') .container .sixteen.columns = render partial: 'admin/shared/flashes', locals: { flashes: } if defined? flashes diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index dd3034ca42..4200cd0176 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -12,11 +12,6 @@ = render partial: 'spree/admin/shared/product_sub_menu' #products_v3_page{ "data-controller": "products", 'data-turbo': true } - .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } - .spinner-container - .spinner - = t('.loading') - = render partial: "content", locals: { products: @products, pagy: @pagy, search_term: @search_term, producer_options: producers, producer_id: @producer_id, category_options: categories, category_id: @category_id, diff --git a/app/webpacker/css/admin_v3/components/spinner.scss b/app/webpacker/css/admin_v3/components/spinner.scss index fd7235ffe3..9f8d8a15e6 100644 --- a/app/webpacker/css/admin_v3/components/spinner.scss +++ b/app/webpacker/css/admin_v3/components/spinner.scss @@ -6,6 +6,11 @@ min-height: 200px; background: rgba(255, 255, 255, 0.8); z-index: 2; + + // Show when inside a loading Turbo Frame + turbo-frame[aria-busy="true"] > & { + display: flex; + } } .spinner-container { @@ -28,7 +33,9 @@ height: 56px; border-radius: 50%; border: 9px solid $teal; - animation: spinner-bulqg1 0.8s infinite linear alternate, spinner-oaa3wk 1.6s infinite linear; + animation: + spinner-bulqg1 0.8s infinite linear alternate, + spinner-oaa3wk 1.6s infinite linear; } } diff --git a/config/locales/en.yml b/config/locales/en.yml index 662b5fe984..2777dab7ad 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -844,6 +844,7 @@ en: index: header: title: Bulk Edit Products + content: loading: Loading your products delete_modal: delete_product_modal: From 1abb068a00e7745958e8b7d0819645a740f8c9ad Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 11:59:52 +1000 Subject: [PATCH 06/13] Enable morphing? I can't really prove if this is working, but it seems to be rendering slightly faster. --- app/views/admin/products_v3/_content.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index 40e22e4bc5..885d851ef4 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -1,4 +1,4 @@ -%turbo-frame#products-content +%turbo-frame#products-content{ refresh: "morph" } .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } .spinner-container .spinner From acc72514de5493c051eab6923869566419628652 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 12:11:17 +1000 Subject: [PATCH 07/13] Fix spec --- spec/system/admin/products_v3/products_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 2afde20c1f..6581ddc657 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -58,7 +58,7 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do select "50", from: "per_page" - expect(page).to have_content "Showing 1 to 50" + expect(page).to have_content "Showing 1 to 50", wait: 10 expect_page_to_be 1 expect_per_page_to_be 50 expect_products_count_to_be 50 @@ -138,7 +138,7 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do search_by_producer "Producer 1" # expect(page).to have_content "1 product found for your search criteria." - expect(page).to have_select "producer_id", selected: "Producer 1" + expect(page).to have_select "producer_id", selected: "Producer 1", wait: 5 expect_products_count_to_be 1 within row_containing_name("Apples") do From 06f67488a9a5098cc365c19f529dbd4a3fd18b08 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 13:08:18 +1000 Subject: [PATCH 08/13] Open links outside of frame by default This page is big enough and it's hard to see how everything works. So links work like links by default (eg edit and clone). Other links and forms are special, and will reload only the frame: this is now explicit in the code. --- app/views/admin/products_v3/_content.html.haml | 2 +- app/views/admin/products_v3/_filters.html.haml | 2 +- app/views/admin/products_v3/_sort.html.haml | 2 +- app/views/admin/products_v3/_table.html.haml | 3 ++- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index 885d851ef4..dcb29e945f 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -1,4 +1,4 @@ -%turbo-frame#products-content{ refresh: "morph" } +%turbo-frame#products-content{ target: "_top", refresh: "morph" } .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } .spinner-container .spinner diff --git a/app/views/admin/products_v3/_filters.html.haml b/app/views/admin/products_v3/_filters.html.haml index 469ec1bf57..2a0aa55428 100644 --- a/app/views/admin/products_v3/_filters.html.haml +++ b/app/views/admin/products_v3/_filters.html.haml @@ -1,4 +1,4 @@ -= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form" } do += form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self" } do = hidden_field_tag :page, nil, class: "page" = hidden_field_tag :per_page, nil, class: "per-page" diff --git a/app/views/admin/products_v3/_sort.html.haml b/app/views/admin/products_v3/_sort.html.haml index 8bd5b249bc..f53eec4e76 100644 --- a/app/views/admin/products_v3/_sort.html.haml +++ b/app/views/admin/products_v3/_sort.html.haml @@ -4,7 +4,7 @@ = t(".pagination.total_html", total: pagy.count, from: pagy.from, to: pagy.to) - if search_term.present? || producer_id.present? || category_id.present? - %a{ href: url_for(page: 1), class: "button disruptive" } + %a{ href: url_for(page: 1), class: "button disruptive", 'data-turbo-frame': "_self" } = t(".pagination.clear_search") %form.with-dropdown diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index e14925b539..1dd4e2f259 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,6 +1,7 @@ = form_with url: bulk_update_admin_products_path, method: :post, id: "products-form", builder: BulkFormBuilder, - html: { data: { controller: "bulk-form", + html: { data: { 'turbo-frame': "_self", + controller: "bulk-form", 'bulk-form-disable-selector-value': "#sort,#filters", 'bulk-form-error-value': defined?(@error_counts), } } do |form| From 1d1169b4788b745305dc25d95566107ae7027620 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 13:08:52 +1000 Subject: [PATCH 09/13] Prevent frame navigations when form is changed This is a hacky hack, filling a gap in Turbo. --- app/views/admin/products_v3/index.html.haml | 2 +- .../controllers/bulk_form_controller.js | 27 ++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index 4200cd0176..48a43e8502 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -1,7 +1,7 @@ - content_for :page_title do = t('.header.title') - content_for :page_actions do - %div{ :class => "toolbar" } + %div{ :class => "toolbar", 'data-turbo': true } %ul{ :class => "actions header-action-links inline-menu" } %li#new_product_link = button_link_to t(:new_product), "/admin/products/new", { :icon => 'icon-plus', :id => 'admin_new_product' } diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 602f7c0127..31000ca9e5 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -33,12 +33,25 @@ export default class BulkFormController extends Controller { this.form.addEventListener("submit", this.#registerSubmit.bind(this)); window.addEventListener("beforeunload", this.preventLeavingChangedForm.bind(this)); + document.addEventListener("turbo:before-visit", this.preventLeavingChangedForm.bind(this)); + + // Frustratingly there's no before-frame-visit, and no other events work, so we need to bind to + // the frame and listen for any link clicks (maybe other things too). + this.turboFrame = this.form.closest("turbo-frame"); + if(this.turboFrame){ + this.turboFrame.addEventListener("click", this.preventLeavingChangedForm.bind(this)) + } } disconnect() { // Make sure to clean up anything that happened outside this.#disableOtherElements(false); window.removeEventListener("beforeunload", this.preventLeavingChangedForm.bind(this)); + document.removeEventListener("turbo:before-visit", this.preventLeavingChangedForm.bind(this)); + + if(this.turboFrame){ + this.turboFrame.removeEventListener("click", this.preventLeavingChangedForm.bind(this)); + } } // Register any new elements (may be called by another controller after dynamically adding fields) @@ -79,9 +92,21 @@ export default class BulkFormController extends Controller { } } - // If form is not being submitted, warn to prevent accidental data loss + // If navigating away from form, warn to prevent accidental data loss preventLeavingChangedForm(event) { + const target = event.target; + // Ignore non-navigation events (see above) + if(this.turboFrame && this.turboFrame.contains(target) && (target.tagName != "A" || target.dataset.turboFrame != "_self")){ + return; + } + if (this.formChanged && !this.submitting) { + // If fired by a custom event (eg Turbo), we need to explicitly ask. + // This is skipped on beforeunload. + if(confirm(I18n.t("are_you_sure"))){ + return; + } + // Cancel the event event.preventDefault(); // Chrome requires returnValue to be set, but ignores the value. Other browsers may display From 91f0a801897a8f8eaa6de7f787cbae258965995b Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 15:05:54 +1000 Subject: [PATCH 10/13] Use POST for action that creates data, duh. Turbo cleverly pre-fetches GET requests to save loading time. But that resulted in dozens of unwanted clones. Attack of the clones!!! I checked: even though this route predates the new products screen, it wasn't being used anywhere else. The old products screen uses the API instead. --- app/views/admin/products_v3/_product_row.html.haml | 2 +- config/routes/spree.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/products_v3/_product_row.html.haml b/app/views/admin/products_v3/_product_row.html.haml index 4a8b0c8ce3..f23d9b0afd 100644 --- a/app/views/admin/products_v3/_product_row.html.haml +++ b/app/views/admin/products_v3/_product_row.html.haml @@ -37,7 +37,7 @@ %td.align-right = render(VerticalEllipsisMenu::Component.new) do = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product) - = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product) + = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product), method: :post %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", "data-modal-link-target-value": "product-delete-modal", "class": "delete", "data-modal-link-modal-dataset-value": {'data-current-id': product.id}.to_json } diff --git a/config/routes/spree.rb b/config/routes/spree.rb index f59e4e7dca..d7c4aa6473 100644 --- a/config/routes/spree.rb +++ b/config/routes/spree.rb @@ -59,7 +59,7 @@ Spree::Core::Engine.routes.draw do resources :products, except: :index do member do - get :clone + post :clone get :group_buy_options get :seo end From 8f9d8b5fb822aca64338ffcf6de41454271e49e5 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 17:26:49 +1000 Subject: [PATCH 11/13] Revert "Prevent frame navigations when form is changed" It was too hacky and had issues. Let's just disable Turbo for those links for now. This reverts commit 1d1169b4788b745305dc25d95566107ae7027620. --- app/views/admin/products_v3/index.html.haml | 2 +- .../controllers/bulk_form_controller.js | 27 +------------------ 2 files changed, 2 insertions(+), 27 deletions(-) diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index 48a43e8502..4200cd0176 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -1,7 +1,7 @@ - content_for :page_title do = t('.header.title') - content_for :page_actions do - %div{ :class => "toolbar", 'data-turbo': true } + %div{ :class => "toolbar" } %ul{ :class => "actions header-action-links inline-menu" } %li#new_product_link = button_link_to t(:new_product), "/admin/products/new", { :icon => 'icon-plus', :id => 'admin_new_product' } diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 31000ca9e5..602f7c0127 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -33,25 +33,12 @@ export default class BulkFormController extends Controller { this.form.addEventListener("submit", this.#registerSubmit.bind(this)); window.addEventListener("beforeunload", this.preventLeavingChangedForm.bind(this)); - document.addEventListener("turbo:before-visit", this.preventLeavingChangedForm.bind(this)); - - // Frustratingly there's no before-frame-visit, and no other events work, so we need to bind to - // the frame and listen for any link clicks (maybe other things too). - this.turboFrame = this.form.closest("turbo-frame"); - if(this.turboFrame){ - this.turboFrame.addEventListener("click", this.preventLeavingChangedForm.bind(this)) - } } disconnect() { // Make sure to clean up anything that happened outside this.#disableOtherElements(false); window.removeEventListener("beforeunload", this.preventLeavingChangedForm.bind(this)); - document.removeEventListener("turbo:before-visit", this.preventLeavingChangedForm.bind(this)); - - if(this.turboFrame){ - this.turboFrame.removeEventListener("click", this.preventLeavingChangedForm.bind(this)); - } } // Register any new elements (may be called by another controller after dynamically adding fields) @@ -92,21 +79,9 @@ export default class BulkFormController extends Controller { } } - // If navigating away from form, warn to prevent accidental data loss + // If form is not being submitted, warn to prevent accidental data loss preventLeavingChangedForm(event) { - const target = event.target; - // Ignore non-navigation events (see above) - if(this.turboFrame && this.turboFrame.contains(target) && (target.tagName != "A" || target.dataset.turboFrame != "_self")){ - return; - } - if (this.formChanged && !this.submitting) { - // If fired by a custom event (eg Turbo), we need to explicitly ask. - // This is skipped on beforeunload. - if(confirm(I18n.t("are_you_sure"))){ - return; - } - // Cancel the event event.preventDefault(); // Chrome requires returnValue to be set, but ignores the value. Other browsers may display From 11541c92700becfe5c0b1f6f52304081d77d150c Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 17:28:12 +1000 Subject: [PATCH 12/13] Disable turbo for those links Now we can warn that "Changes that you made might not be saved" --- app/views/admin/products_v3/_product_row.html.haml | 4 ++-- app/views/admin/products_v3/_table.html.haml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/admin/products_v3/_product_row.html.haml b/app/views/admin/products_v3/_product_row.html.haml index f23d9b0afd..ed1063bc3a 100644 --- a/app/views/admin/products_v3/_product_row.html.haml +++ b/app/views/admin/products_v3/_product_row.html.haml @@ -36,8 +36,8 @@ .content= product.inherits_properties ? 'YES' : 'NO' #TODO: consider using https://github.com/RST-J/human_attribute_values, else use I18n.t (also below) %td.align-right = render(VerticalEllipsisMenu::Component.new) do - = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product) - = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product), method: :post + = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product), 'data-turbo': false + = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product), method: :post, 'data-turbo': false %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", "data-modal-link-target-value": "product-delete-modal", "class": "delete", "data-modal-link-modal-dataset-value": {'data-current-id': product.id}.to_json } diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 1dd4e2f259..78d32be8b2 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -44,7 +44,7 @@ -# Y products could not be saved correctly. Please review errors and try again = t('.error_summary.invalid', count: @error_counts[:invalid]) .form-buttons - %a.button.reset.medium{ href: admin_products_path(page: @page, per_page: @per_page, search_term: @search_term, producer_id: @producer_id, category_id: @category_id) } + %a.button.reset.medium{ href: admin_products_path(page: @page, per_page: @per_page, search_term: @search_term, producer_id: @producer_id, category_id: @category_id), 'data-turbo': "false" } = t('.reset') = form.submit t('.save'), class: "medium" %tr From a2255e62d42690fcf5a3562402c11a8fccf19e27 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 17 Apr 2024 17:30:03 +1000 Subject: [PATCH 13/13] Revert "Use POST for action that creates data," I'm not happy about it, but we need it to be a standard link to make it work. I assume it's because BulkFormController.preventLeavingChangedForm() isn't smart enough. This reverts commit 91f0a801897a8f8eaa6de7f787cbae258965995b. --- app/views/admin/products_v3/_product_row.html.haml | 2 +- config/routes/spree.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/products_v3/_product_row.html.haml b/app/views/admin/products_v3/_product_row.html.haml index ed1063bc3a..5e11301a5e 100644 --- a/app/views/admin/products_v3/_product_row.html.haml +++ b/app/views/admin/products_v3/_product_row.html.haml @@ -37,7 +37,7 @@ %td.align-right = render(VerticalEllipsisMenu::Component.new) do = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product), 'data-turbo': false - = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product), method: :post, 'data-turbo': false + = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product), 'data-turbo': false %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", "data-modal-link-target-value": "product-delete-modal", "class": "delete", "data-modal-link-modal-dataset-value": {'data-current-id': product.id}.to_json } diff --git a/config/routes/spree.rb b/config/routes/spree.rb index d7c4aa6473..f59e4e7dca 100644 --- a/config/routes/spree.rb +++ b/config/routes/spree.rb @@ -59,7 +59,7 @@ Spree::Core::Engine.routes.draw do resources :products, except: :index do member do - post :clone + get :clone get :group_buy_options get :seo end