From 4467758a9f96152ca2386187b6dc091e17725e71 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 21 Sep 2023 09:33:41 +1000 Subject: [PATCH 1/5] Apply code suggestion Co-authored-by: Jean-Baptiste Bellet --- app/webpacker/controllers/bulk_form_controller.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 3c98a5ce65..9b959d1d93 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -43,11 +43,9 @@ export default class BulkFormController extends Controller { toggleFormModified() { // For each record, check if any fields are modified - const modifiedRecordCount = Object.keys(this.recordElements).filter((recordId) => { - return this.recordElements[recordId].some((element) => { - return element.value != element.defaultValue; - }); - }).length; + const modifiedRecordCount = Object.values(this.recordElements).filter((elements) => + elements.some((element) => element.value != element.defaultValue) + ).length; const formModified = modifiedRecordCount > 0; // Show actions From ed207e3df64ff1d467d48d5b830a844c514f281d Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 21 Sep 2023 09:37:46 +1000 Subject: [PATCH 2/5] DRY up code --- app/webpacker/controllers/bulk_form_controller.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 9b959d1d93..526aa1d5b5 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -35,8 +35,7 @@ export default class BulkFormController extends Controller { toggleModified(e) { const element = e.target; - const modified = element.value != element.defaultValue; - element.classList.toggle("modified", modified); + element.classList.toggle("modified", this.#isModified(element)); this.toggleFormModified(); } @@ -44,7 +43,7 @@ export default class BulkFormController extends Controller { toggleFormModified() { // For each record, check if any fields are modified const modifiedRecordCount = Object.values(this.recordElements).filter((elements) => - elements.some((element) => element.value != element.defaultValue) + elements.some(this.#isModified) ).length; const formModified = modifiedRecordCount > 0; @@ -85,4 +84,8 @@ export default class BulkFormController extends Controller { }); } } + + #isModified(element) { + return element.value != element.defaultValue; + } } From cd63ab63d88fdb708cb9b127681bdbbf2af9042d Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 19 Sep 2023 15:40:40 +1000 Subject: [PATCH 3/5] Refactor form code Co-authored-by: Maikel --- app/views/admin/products_v3/_table.html.haml | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index b463b93adc..b64072a270 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -63,18 +63,17 @@ %td.align-right = render partial: 'admin/products_v3/components/product_actions', locals: { product: product } - product.variants.each do |variant| - - prefix = "[products][][variants_attributes][]" # Couldn't convince the formbuilder to generate this for me, so for now we manually add the prefix - = form.fields_for(variant) do |variant_form| + = form.fields_for("products][][variants_attributes][", variant, index: nil) do |variant_form| %tr.condensed %td.align-left - = variant_form.hidden_field :id, name: "#{prefix}[id]" - = variant_form.text_field :display_name, name: "#{prefix}[display_name]", 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name + = variant_form.hidden_field :id + = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name %td.align-right - = variant_form.text_field :sku, name: "#{prefix}[sku]", 'aria-label': t('admin.products_page.columns.sku') + = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku') %td.align-right .line-clamp-1= variant.unit_to_display %td.align-right - = variant_form.text_field :price, name: "#{prefix}[price]", 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs + = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs %td.align-right .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements. %td.align-left From 6d35b1ac716f246c2b3f6316f2f79b5c7ca39a59 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 20 Sep 2023 11:43:08 +1000 Subject: [PATCH 4/5] Fix bulk form input styles 'header' fields are meant to be bold, and the field backgrounds are meant to match the cell background colour. --- app/webpacker/css/admin/products_v3.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index 434c20fce3..da512aecf7 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -109,6 +109,10 @@ // "Naked" inputs. Row hover helps reveal them. input { border-color: transparent; + background-color: $color-tbl-cell-bg; + height: auto; + font-size: inherit; + font-weight: inherit; &:focus { border-color: $color-txt-hover-brd; From 6f43165006d421838ce54555176f27296a169cfb Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 20 Sep 2023 14:13:22 +1000 Subject: [PATCH 5/5] Show generic error message when StimulusReflex fails I decided not to invest the time to figure out how to test this.. The messages should have the same effect as the 500 error, but not technically a 500 because it's not handling a HTTP response. The documentation seems to state this covers server-side errors only (not network errors for example). I couldn't find any way to handle network errors with action cable. --- app/webpacker/controllers/application_controller.js | 4 ++++ config/locales/en.yml | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/app/webpacker/controllers/application_controller.js b/app/webpacker/controllers/application_controller.js index 462456ff01..3065427cf4 100644 --- a/app/webpacker/controllers/application_controller.js +++ b/app/webpacker/controllers/application_controller.js @@ -43,7 +43,11 @@ export default class extends Controller { } reflexError(element, reflex, error, reflexId) { + // Log to console (it normally only gets logged in dev mode) + console.error(reflex + ":\n " + error); + // show error message + alert(I18n.t("errors.stimulus_reflex_error")); } reflexForbidden(element, reflex, noop, reflexId) { diff --git a/config/locales/en.yml b/config/locales/en.yml index b279f8f91b..95ea7a2e0a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -165,6 +165,14 @@ en: message_html: "

The change you wanted was rejected. Maybe you tried to change something you don't have access to.

Return home

" + stimulus_reflex_error: "We're sorry, but something went wrong. + + + This might be a temporary problem, so please try again or reload the page. + + We record all errors and may be working on a fix. + + If the problem persists or is urgent, please contact us." stripe: error_code: incorrect_number: "The card number is incorrect."