From 18e40bebd0939162bd188600b2b2090b6bb872bd Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 7 Sep 2023 16:27:52 +1000 Subject: [PATCH 1/9] Mark modified fields --- app/views/admin/products_v3/_table.html.haml | 4 +- .../controllers/bulk_form_controller.js | 21 ++++++++ app/webpacker/css/admin/products_v3.scss | 4 ++ .../css/admin_v3/globals/variables.scss | 1 + app/webpacker/css/admin_v3/shared/forms.scss | 4 ++ .../stimulus/bulk_form_controller_test.js | 49 +++++++++++++++++++ 6 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 app/webpacker/controllers/bulk_form_controller.js create mode 100644 spec/javascripts/stimulus/bulk_form_controller_test.js diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 7c3aa22d5a..a200a8cd41 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_v3_index_path, method: :patch, id: "products-form", - - html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update'} do |form| + html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', + 'data-controller': "bulk-form"} do |form| %fieldset.form-actions .container .status.ten.columns diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js new file mode 100644 index 0000000000..163e46cde1 --- /dev/null +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -0,0 +1,21 @@ +import { Controller } from "stimulus"; + +// Manages "modified" state for a form with multiple records +export default class BulkFormController extends Controller { + connect() { + this.form = this.element; + + // Start listening for any changes within the form + // this.element.addEventListener('change', this.toggleModified.bind(this)); // dunno why this doesn't work + for (const element of this.form.elements) { + element.addEventListener("keyup", this.toggleModified.bind(this)); // instant response + element.addEventListener("change", this.toggleModified.bind(this)); // just in case (eg right-click paste) + } + } + + toggleModified(e) { + const element = e.target; + const changed = element.value != element.defaultValue; + element.classList.toggle("modified", changed); + } +} diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index c673cbe139..47ef952974 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -100,6 +100,10 @@ &:focus { border-color: $color-txt-hover-brd; } + + &.modified { + border-color: $color-txt-modified-brd; + } } .field_with_errors { diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss index 7706a4073d..b30f7ac140 100644 --- a/app/webpacker/css/admin_v3/globals/variables.scss +++ b/app/webpacker/css/admin_v3/globals/variables.scss @@ -74,6 +74,7 @@ $color-sel-hover-text: $white !default; $color-txt-brd: $color-border !default; $color-txt-text: $near-black !default; $color-txt-hover-brd: $teal !default; +$color-txt-modified-brd: $bright-orange !default; $vpadding-txt: 5px; $hpadding-txt: 8px; diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss index b9abfd6068..570fa95c16 100644 --- a/app/webpacker/css/admin_v3/shared/forms.scss +++ b/app/webpacker/css/admin_v3/shared/forms.scss @@ -21,6 +21,10 @@ fieldset { &[disabled] { opacity: 0.7; } + + &.modified { + border-color: $color-txt-modified-brd; + } } textarea { diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js new file mode 100644 index 0000000000..0a2ce229da --- /dev/null +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -0,0 +1,49 @@ +/** + * @jest-environment jsdom + */ + +import { Application } from "stimulus"; +import bulk_form_controller from "../../../app/webpacker/controllers/bulk_form_controller"; + +describe("BulkFormController", () => { + beforeAll(() => { + const application = Application.start(); + application.register("bulk-form", bulk_form_controller); + }); + + beforeEach(() => { + document.body.innerHTML = ` +
+ + +
+ `; + }); + + describe("#toggleModified", () => { + it("marks a changed element as modified", () => { + // const form = document.getElementsByTagName("form")[0]; + const input1 = document.getElementById("input1"); + const input2 = document.getElementById("input2"); + + expect(input1.classList).not.toContain('modified'); + expect(input2.classList).not.toContain('modified'); + + // Value has been changed (we're not simulating a user in a browser here; we're testing DOM events directly) + input1.value = 'updated1'; + input1.dispatchEvent(new Event("change")); + // form.dispatchEvent(new Event("change")); + + expect(input1.classList).toContain('modified'); + expect(input2.classList).not.toContain('modified'); + + // Change back to original value + input1.value = 'initial1'; + input1.dispatchEvent(new Event("change")); + // form.dispatchEvent(new Event("change")); + + expect(input1.classList).not.toContain('modified'); + expect(input2.classList).not.toContain('modified'); + }); + }); +}); From daefada5a9b9fc9ab05e5122863bea5c5f62165d Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 8 Sep 2023 13:12:48 +1000 Subject: [PATCH 2/9] Show summary of modified records I found myself trying to write Ruby in Javascript, and it's not nearly as pretty.. Javascript now has more advanced data structures like Map, but it's rather useless because it doesn't have the usual iterator methods (such as filter, map, reduce etc). Also for the spec I wasn't sure of the best approach, so will gladly recieve feedback. --- app/views/admin/products_v3/_table.html.haml | 6 +- .../controllers/bulk_form_controller.js | 32 ++++++- config/locales/en.yml | 4 + .../stimulus/bulk_form_controller_test.js | 96 +++++++++++++++---- 4 files changed, 115 insertions(+), 23 deletions(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index a200a8cd41..8e54f978ac 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,10 +1,10 @@ = form_with url: bulk_update_admin_products_v3_index_path, method: :patch, id: "products-form", html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', 'data-controller': "bulk-form"} do |form| - %fieldset.form-actions + %fieldset.form-actions{ 'data-bulk-form-target': "actions" } .container .status.ten.columns - / = t('.products_modified', count: 'X') + .modified_summary{ 'data-bulk-form-target': "modifiedSummary", 'data-translation-key': 'admin.products_v3.table.modified_summary'} - if defined?(error_msg) && error_msg.present? .error = error_msg @@ -34,7 +34,7 @@ %th.align-left= t('admin.products_page.columns.inherits_properties') - products.each do |product| = form.fields_for("products", product, index: nil) do |product_form| - %tbody.relaxed + %tbody.relaxed{ 'data-record-id': product_form.object.id } %tr %td.align-left.header = product_form.hidden_field :id diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 163e46cde1..d812f907dc 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -2,6 +2,9 @@ import { Controller } from "stimulus"; // Manages "modified" state for a form with multiple records export default class BulkFormController extends Controller { + static targets = ["actions", "modifiedSummary"]; + recordElements = {}; + connect() { this.form = this.element; @@ -10,12 +13,37 @@ export default class BulkFormController extends Controller { for (const element of this.form.elements) { element.addEventListener("keyup", this.toggleModified.bind(this)); // instant response element.addEventListener("change", this.toggleModified.bind(this)); // just in case (eg right-click paste) + + // Set up a tree of fields according to their associated record + const recordContainer = element.closest("[data-record-id]"); // The JS could be more efficient if this data was added to each element. But I didn't want to pollute the HTML too much. + const recordId = recordContainer && recordContainer.dataset.recordId; + if (recordId) { + this.recordElements[recordId] ||= []; + this.recordElements[recordId].push(element); + } } } toggleModified(e) { const element = e.target; - const changed = element.value != element.defaultValue; - element.classList.toggle("modified", changed); + const modified = element.value != element.defaultValue; + element.classList.toggle("modified", modified); + + this.toggleFormModified(); + } + + 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; + + // Display number of records modified + const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey; + if (key) { + this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount }); + } } } diff --git a/config/locales/en.yml b/config/locales/en.yml index 245f23ea60..d966cd21c1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -818,6 +818,10 @@ en: import_products: Import multiple products no_products_found_for_search: No products found for your search criteria table: + modified_summary: + zero: "" + one: "%{count} product modified." + other: "%{count} products modified." save: Save changes reset: Discard changes bulk_update: # TODO: fix these diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index 0a2ce229da..7134251b0e 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -11,39 +11,99 @@ describe("BulkFormController", () => { application.register("bulk-form", bulk_form_controller); }); + // Mock I18n. TODO: moved to a shared helper + beforeAll(() => { + const mockedT = jest.fn(); + mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts))); + + global.I18n = { + t: mockedT + }; + }) + + // (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 ) + afterAll(() => { + delete global.I18n; + }) + beforeEach(() => { document.body.innerHTML = `
- - +
+
+ + +
+
+ +
`; }); - describe("#toggleModified", () => { - it("marks a changed element as modified", () => { - // const form = document.getElementsByTagName("form")[0]; - const input1 = document.getElementById("input1"); + describe("Modifying input values", () => { + // This is more of a behaviour spec. Jest doesn't have all the niceties of RSpec so lots of code + // would be repeated if these were broken into multiple examples. So it seems impractical to + // write individual unit tests. + it("counts modified fields and records", () => { + const modified_summary = document.getElementById("modified_summary"); + const input1a = document.getElementById("input1a"); + const input1b = document.getElementById("input1b"); const input2 = document.getElementById("input2"); - expect(input1.classList).not.toContain('modified'); + // Record 1: First field changed (we're not simulating a user in a browser here; we're testing DOM events directly) + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + // Expect only first field to show modified, and show modified summary translation + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); - // Value has been changed (we're not simulating a user in a browser here; we're testing DOM events directly) - input1.value = 'updated1'; - input1.dispatchEvent(new Event("change")); - // form.dispatchEvent(new Event("change")); - - expect(input1.classList).toContain('modified'); + // Record 1: Second field changed + input1b.value = 'updated1b'; + input1b.dispatchEvent(new Event("change")); + // Expect to show modified, and same summary translation + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); - // Change back to original value - input1.value = 'initial1'; - input1.dispatchEvent(new Event("change")); - // form.dispatchEvent(new Event("change")); + // Record 2: has been changed + input2.value = 'updated2'; + input2.dispatchEvent(new Event("change")); + // Expect all fields to show modified, summary counts both records + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); - expect(input1.classList).not.toContain('modified'); + // Record 1: Change first field back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + // Expect first field to not show modified. But both records are still modified. + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + + // Record 1: Change second field back to original value + input1b.value = 'initial1b'; + input1b.dispatchEvent(new Event("change")); + // Both fields for record 1 show unmodified, but second record is still modified + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + + // Record 2: Change back to original value + input2.value = 'initial2'; + input2.dispatchEvent(new Event("change")); + // No fields or records are modified + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); }); }); }); From 15f7a8299b5d0118a8f436d4df110a5e81a27e21 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 8 Sep 2023 12:56:55 +1000 Subject: [PATCH 3/9] Show form actions only when modified --- app/views/admin/products_v3/_table.html.haml | 2 +- app/webpacker/controllers/bulk_form_controller.js | 2 ++ spec/javascripts/stimulus/bulk_form_controller_test.js | 8 ++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 8e54f978ac..7e94f84c32 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,7 +1,7 @@ = form_with url: bulk_update_admin_products_v3_index_path, method: :patch, id: "products-form", html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', 'data-controller': "bulk-form"} do |form| - %fieldset.form-actions{ 'data-bulk-form-target': "actions" } + %fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" } .container .status.ten.columns .modified_summary{ 'data-bulk-form-target': "modifiedSummary", 'data-translation-key': 'admin.products_v3.table.modified_summary'} diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index d812f907dc..d008ad7485 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -40,6 +40,8 @@ export default class BulkFormController extends Controller { }); }).length; + this.actionsTarget.classList.toggle("hidden", modifiedRecordCount == 0); + // Display number of records modified const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey; if (key) { diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index 7134251b0e..a5ef778926 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -29,6 +29,7 @@ describe("BulkFormController", () => { beforeEach(() => { document.body.innerHTML = `
+
@@ -46,6 +47,7 @@ describe("BulkFormController", () => { // would be repeated if these were broken into multiple examples. So it seems impractical to // write individual unit tests. it("counts modified fields and records", () => { + const actions = document.getElementById("actions"); const modified_summary = document.getElementById("modified_summary"); const input1a = document.getElementById("input1a"); const input1b = document.getElementById("input1b"); @@ -58,6 +60,7 @@ describe("BulkFormController", () => { expect(input1a.classList).toContain('modified'); expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); // Record 1: Second field changed @@ -67,6 +70,7 @@ describe("BulkFormController", () => { expect(input1a.classList).toContain('modified'); expect(input1b.classList).toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); // Record 2: has been changed @@ -76,6 +80,7 @@ describe("BulkFormController", () => { expect(input1a.classList).toContain('modified'); expect(input1b.classList).toContain('modified'); expect(input2.classList).toContain('modified'); + expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); // Record 1: Change first field back to original value @@ -85,6 +90,7 @@ describe("BulkFormController", () => { expect(input1a.classList).not.toContain('modified'); expect(input1b.classList).toContain('modified'); expect(input2.classList).toContain('modified'); + expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); // Record 1: Change second field back to original value @@ -94,6 +100,7 @@ describe("BulkFormController", () => { expect(input1a.classList).not.toContain('modified'); expect(input1b.classList).not.toContain('modified'); expect(input2.classList).toContain('modified'); + expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); // Record 2: Change back to original value @@ -103,6 +110,7 @@ describe("BulkFormController", () => { expect(input1a.classList).not.toContain('modified'); expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(actions.classList).toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); }); }); From e047f49998bdb007ade51dd7b156e79f4520b348 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 13 Sep 2023 15:51:09 +1000 Subject: [PATCH 4/9] Make form actions float over sort controls. --- app/views/admin/products_v3/_content.html.haml | 4 ++-- app/webpacker/css/admin/products_v3.scss | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index c9f33c55ef..0cda71ff9a 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -7,10 +7,10 @@ category_options: category_options, category_id: category_id } - if products.any? - .container + .container.results .sixteen.columns = render partial: 'sort', locals: { pagy: pagy, search_term: search_term, producer_id: producer_id, category_id: category_id } - = render partial: 'table', locals: { products: products } + = render partial: 'table', locals: { products: products } - if pagy.pages > 1 = render partial: 'admin/shared/v3/pagy', locals: { pagy: pagy, reflex: "click->Products#fetch" } - else diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index 47ef952974..fb8cb82300 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -15,6 +15,18 @@ padding: 0; } + .results { + position: relative; + } + + // Form actions floats over other controls when active + .form-actions { + position: absolute; + top: 1em; + left: 0; + right: 0; + } + // Hopefully these rules will be moved to component(s). table.products { table-layout: fixed; // Column widths are based solely on col definitions (not content). This allows more efficient rendering. From 8ff67aca415a6e669540192c540bf510f0792b7f Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 14 Sep 2023 11:12:13 +1000 Subject: [PATCH 5/9] Disable filters and sorting when form is modified Stimulus controllers aren't supposed to reach outside their own element (so we can't do this with targets). Perhaps the controller should be bigger to encompass more, but I wanted to see if I could avoid making a mega component that does everything. For now it seems appropriate just to pass a selector in. Another option is to publish events on other controllers using Outlets, but I don't know if we need to go there just yet. --- app/views/admin/products_v3/_table.html.haml | 2 +- .../controllers/bulk_form_controller.js | 25 +++++++++++- app/webpacker/css/admin/products_v3.scss | 21 +++++++++- .../stimulus/bulk_form_controller_test.js | 40 ++++++++++++++++++- 4 files changed, 83 insertions(+), 5 deletions(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 7e94f84c32..4d31694077 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_v3_index_path, method: :patch, id: "products-form", html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', - 'data-controller': "bulk-form"} do |form| + 'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters"} do |form| %fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" } .container .status.ten.columns diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index d008ad7485..d5a70a9105 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -3,6 +3,9 @@ import { Controller } from "stimulus"; // Manages "modified" state for a form with multiple records export default class BulkFormController extends Controller { static targets = ["actions", "modifiedSummary"]; + static values = { + disableSelector: String, + }; recordElements = {}; connect() { @@ -24,6 +27,11 @@ export default class BulkFormController extends Controller { } } + disconnect() { + // Make sure to clean up anything that happened outside + this.#disableOtherElements(false); + } + toggleModified(e) { const element = e.target; const modified = element.value != element.defaultValue; @@ -39,8 +47,11 @@ export default class BulkFormController extends Controller { return element.value != element.defaultValue; }); }).length; + const formModified = modifiedRecordCount > 0; - this.actionsTarget.classList.toggle("hidden", modifiedRecordCount == 0); + // Show actions + this.actionsTarget.classList.toggle("hidden", !formModified); + this.#disableOtherElements(formModified); // like filters and sorting // Display number of records modified const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey; @@ -48,4 +59,16 @@ export default class BulkFormController extends Controller { this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount }); } } + + // private + + #disableOtherElements(disable) { + this.disableElements ||= document.querySelectorAll(this.disableSelectorValue); + + if (this.disableElements) { + this.disableElements.forEach((element) => { + element.classList.toggle("disabled-section", disable); + }); + } + } } diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index fb8cb82300..434c20fce3 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -22,9 +22,10 @@ // Form actions floats over other controls when active .form-actions { position: absolute; - top: 1em; + top: -1em; left: 0; right: 0; + z-index: 1; // Ensure tom-select and .disabled-section are covered } // Hopefully these rules will be moved to component(s). @@ -234,4 +235,22 @@ } } } + + // Blurred and non-clickable + $disabled-blur: 1.5px; + .disabled-section { + position: relative; + + &::after { + content: ""; + position: absolute; + backdrop-filter: blur($disabled-blur); + // Stretch outside for a soft blur edge: + left: -$disabled-blur; + top: -$disabled-blur; + bottom: -$disabled-blur; + right: -$disabled-blur; + z-index: 1; // Ensure tom-select is covered + } + } } diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index a5ef778926..ce1d0bda0d 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -28,7 +28,9 @@ describe("BulkFormController", () => { beforeEach(() => { document.body.innerHTML = ` - +
+
+
@@ -38,6 +40,7 @@ describe("BulkFormController", () => {
+ `; }); @@ -47,6 +50,8 @@ describe("BulkFormController", () => { // would be repeated if these were broken into multiple examples. So it seems impractical to // write individual unit tests. it("counts modified fields and records", () => { + const disable1 = document.getElementById("disable1"); + const disable2 = document.getElementById("disable2"); const actions = document.getElementById("actions"); const modified_summary = document.getElementById("modified_summary"); const input1a = document.getElementById("input1a"); @@ -56,12 +61,15 @@ describe("BulkFormController", () => { // Record 1: First field changed (we're not simulating a user in a browser here; we're testing DOM events directly) input1a.value = 'updated1a'; input1a.dispatchEvent(new Event("change")); - // Expect only first field to show modified, and show modified summary translation + // Expect only first field to show modified expect(input1a.classList).toContain('modified'); expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + // Actions and modified summary are shown, with other sections disabled expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(disable1.classList).toContain('disabled-section'); + expect(disable2.classList).toContain('disabled-section'); // Record 1: Second field changed input1b.value = 'updated1b'; @@ -102,6 +110,8 @@ describe("BulkFormController", () => { expect(input2.classList).toContain('modified'); expect(actions.classList).not.toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(disable1.classList).toContain('disabled-section'); + expect(disable2.classList).toContain('disabled-section'); // Record 2: Change back to original value input2.value = 'initial2'; @@ -110,8 +120,34 @@ describe("BulkFormController", () => { expect(input1a.classList).not.toContain('modified'); expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + // Actions are hidden and other sections are now re-enabled expect(actions.classList).toContain('hidden'); expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); + expect(disable1.classList).not.toContain('disabled-section'); + expect(disable2.classList).not.toContain('disabled-section'); }); }); + + // unable to test disconnect at this stage + // describe("disconnect()", () => { + // it("resets other elements", () => { + // const disable1 = document.getElementById("disable1"); + // const disable2 = document.getElementById("disable2"); + // const controller = document.querySelector('[data-controller="bulk-form"]'); + // const form = document.querySelector('[data-controller="bulk-form"]'); + + // // Form is modified and other sections are disabled + // input1a.value = 'updated1a'; + // input1a.dispatchEvent(new Event("change")); + // expect(disable1.classList).toContain('disabled-section'); + // expect(disable2.classList).toContain('disabled-section'); + + // // form.submit(); //not implemented + // document.body.removeChild(controller); //doesn't trigger disconnect + // controller.innerHTML = ''; //doesn't trigger disconnect + + // expect(disable1.classList).not.toContain('disabled-section'); + // expect(disable2.classList).not.toContain('disabled-section'); + // }); + // }); }); From 99ac48a258e4db9222f2c9cbb3c13ab3f19d7d44 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 15 Sep 2023 15:38:14 +1000 Subject: [PATCH 6/9] Discarding changes reloads from DB But it also clears any search filters. To confirm exactly what behaviour is desired before fixing... --- app/views/admin/products_v3/_table.html.haml | 2 +- .../system/admin/products_v3/products_spec.rb | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 4d31694077..95dbf74398 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -9,7 +9,7 @@ .error = error_msg .form-buttons.six.columns - = form.submit t('.reset'), type: :reset, class: "medium" + = form.submit t('.reset'), type: :reset, class: "medium", 'data-reflex': 'click->products#fetch' = form.submit t('.save'), class: "medium" %table.products %col{ width:"15%" } diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 0a0c83cbe4..80f2bc19f0 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -205,6 +205,25 @@ describe 'As an admin, I can see the new product page' do pending expect(page).to have_content "Changes saved" end + + it "can discard changes and reload latest data" do + within row_containing_name("Apples") do + fill_in "Name", with: "Pommes" + end + + # Meanwhile, the SKU was updated + product_a.update! sku: "APL-10" + + expect { + click_button "Discard changes" + product_a.reload + }.to_not change { product_a.name } + + within row_containing_name("Apples") do + expect(page).to have_field "Name", with: "Apples" # Changed value wasn't saved + expect(page).to have_field "SKU", with: "APL-10" # Updated value shown + end + end end def expect_page_to_be(page_number) From e075d405257a8a70742da2bf81b22e358536a821 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 15 Sep 2023 12:53:55 +1000 Subject: [PATCH 7/9] Prevent accidentally leaving the page --- .../controllers/bulk_form_controller.js | 16 ++++++++++++++++ .../stimulus/bulk_form_controller_test.js | 1 + spec/system/admin/products_v3/products_spec.rb | 8 ++++++++ 3 files changed, 25 insertions(+) diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index d5a70a9105..3c98a5ce65 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -30,6 +30,7 @@ export default class BulkFormController extends Controller { disconnect() { // Make sure to clean up anything that happened outside this.#disableOtherElements(false); + window.removeEventListener("beforeunload", this.preventLeavingBulkForm); } toggleModified(e) { @@ -58,6 +59,21 @@ export default class BulkFormController extends Controller { if (key) { this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount }); } + + // Prevent accidental data loss + if (formModified) { + window.addEventListener("beforeunload", this.preventLeavingBulkForm); + } else { + window.removeEventListener("beforeunload", this.preventLeavingBulkForm); + } + } + + preventLeavingBulkForm(e) { + // Cancel the event + e.preventDefault(); + // Chrome requires returnValue to be set. Other browsers may display this if provided, but let's + // not create a new translation key, and keep the behaviour consistent. + e.returnValue = ""; } // private diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index ce1d0bda0d..8375f51eb0 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -148,6 +148,7 @@ describe("BulkFormController", () => { // expect(disable1.classList).not.toContain('disabled-section'); // expect(disable2.classList).not.toContain('disabled-section'); + // //TODO: expect window to have no beforeunload event listener // }); // }); }); diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 80f2bc19f0..598395645b 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -211,6 +211,14 @@ describe 'As an admin, I can see the new product page' do fill_in "Name", with: "Pommes" end + # Expect to be alerted when attempting to navigate away. Cancel. + dismiss_confirm do + click_link "Dashboard" + end + within row_containing_name("Apples") do + expect(page).to have_field "Name", with: "Pommes" # Changed value wasn't lost + end + # Meanwhile, the SKU was updated product_a.update! sku: "APL-10" From 6bfdd1bc12b504a9934ea7802bc2d5a92c7c4259 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 19 Sep 2023 14:46:03 +1000 Subject: [PATCH 8/9] Split spec into multiple examples --- .../stimulus/bulk_form_controller_test.js | 152 ++++++++++-------- 1 file changed, 86 insertions(+), 66 deletions(-) diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index 8375f51eb0..1e706f7476 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -46,10 +46,7 @@ describe("BulkFormController", () => { }); describe("Modifying input values", () => { - // This is more of a behaviour spec. Jest doesn't have all the niceties of RSpec so lots of code - // would be repeated if these were broken into multiple examples. So it seems impractical to - // write individual unit tests. - it("counts modified fields and records", () => { + beforeEach(() => { const disable1 = document.getElementById("disable1"); const disable2 = document.getElementById("disable2"); const actions = document.getElementById("actions"); @@ -57,74 +54,97 @@ describe("BulkFormController", () => { const input1a = document.getElementById("input1a"); const input1b = document.getElementById("input1b"); const input2 = document.getElementById("input2"); + }); - // Record 1: First field changed (we're not simulating a user in a browser here; we're testing DOM events directly) - input1a.value = 'updated1a'; - input1a.dispatchEvent(new Event("change")); - // Expect only first field to show modified - expect(input1a.classList).toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).not.toContain('modified'); - // Actions and modified summary are shown, with other sections disabled - expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); - expect(disable1.classList).toContain('disabled-section'); - expect(disable2.classList).toContain('disabled-section'); + describe("marking changed fields", () => { + it("onChange", () => { + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + // Expect only first field to show modified + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).not.toContain('modified'); - // Record 1: Second field changed - input1b.value = 'updated1b'; - input1b.dispatchEvent(new Event("change")); - // Expect to show modified, and same summary translation - expect(input1a.classList).toContain('modified'); - expect(input1b.classList).toContain('modified'); - expect(input2.classList).not.toContain('modified'); - expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + // Change back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + expect(input1a.classList).not.toContain('modified'); - // Record 2: has been changed - input2.value = 'updated2'; - input2.dispatchEvent(new Event("change")); - // Expect all fields to show modified, summary counts both records - expect(input1a.classList).toContain('modified'); - expect(input1b.classList).toContain('modified'); - expect(input2.classList).toContain('modified'); - expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + }); - // Record 1: Change first field back to original value - input1a.value = 'initial1a'; - input1a.dispatchEvent(new Event("change")); - // Expect first field to not show modified. But both records are still modified. - expect(input1a.classList).not.toContain('modified'); - expect(input1b.classList).toContain('modified'); - expect(input2.classList).toContain('modified'); - expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + it("multiple fields", () => { + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + input2.value = 'updated2'; + input2.dispatchEvent(new Event("change")); + // Expect only first field to show modified + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).toContain('modified'); - // Record 1: Change second field back to original value - input1b.value = 'initial1b'; - input1b.dispatchEvent(new Event("change")); - // Both fields for record 1 show unmodified, but second record is still modified - expect(input1a.classList).not.toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).toContain('modified'); - expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); - expect(disable1.classList).toContain('disabled-section'); - expect(disable2.classList).toContain('disabled-section'); + // Change only one back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).toContain('modified'); + }); + }) - // Record 2: Change back to original value - input2.value = 'initial2'; - input2.dispatchEvent(new Event("change")); - // No fields or records are modified - expect(input1a.classList).not.toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).not.toContain('modified'); - // Actions are hidden and other sections are now re-enabled - expect(actions.classList).toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); - expect(disable1.classList).not.toContain('disabled-section'); - expect(disable2.classList).not.toContain('disabled-section'); + describe("activating sections, and showing a summary", () => { + // This scenario should probably be broken up into smaller units. + it("counts modified records ", () => { + // Record 1: First field changed + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + // Actions and modified summary are shown, with other sections disabled + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(disable1.classList).toContain('disabled-section'); + expect(disable2.classList).toContain('disabled-section'); + + // Record 1: Second field changed + input1b.value = 'updated1b'; + input1b.dispatchEvent(new Event("change")); + // Expect to show same summary translation + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + + // Record 2: has been changed + input2.value = 'updated2'; + input2.dispatchEvent(new Event("change")); + // Expect summary to count both records + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + + // Record 1: Change first field back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + // Both records are still modified. + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + + // Record 1: Change second field back to original value + input1b.value = 'initial1b'; + input1b.dispatchEvent(new Event("change")); + // Both fields for record 1 show unmodified, but second record is still modified + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(disable1.classList).toContain('disabled-section'); + expect(disable2.classList).toContain('disabled-section'); + + // Record 2: Change back to original value + input2.value = 'initial2'; + input2.dispatchEvent(new Event("change")); + // Actions are hidden and other sections are now re-enabled + expect(actions.classList).toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); + expect(disable1.classList).not.toContain('disabled-section'); + expect(disable2.classList).not.toContain('disabled-section'); + }); }); }); From 759705efcf8f3f64b57697fb7ea21ddd8a1dcaba Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 19 Sep 2023 14:46:55 +1000 Subject: [PATCH 9/9] Add spec for onKeyup --- .../stimulus/bulk_form_controller_test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index 1e706f7476..cc31531950 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -72,6 +72,20 @@ describe("BulkFormController", () => { }); + it("onKeyup", () => { + input1a.value = 'u1a'; + input1a.dispatchEvent(new Event("keyup")); + // Expect only first field to show modified + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).not.toContain('modified'); + + // Change back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("keyup")); + expect(input1a.classList).not.toContain('modified'); + }); + it("multiple fields", () => { input1a.value = 'updated1a'; input1a.dispatchEvent(new Event("change"));