From da69e2c383df45c5120f99cee0c5f3acfeae29d2 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 23 Mar 2026 15:14:16 +1100 Subject: [PATCH 1/8] Widen action menus slightly when needed --- .../vertical_ellipsis_menu_component.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss index dd698ee331..75e5e6da50 100644 --- a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss +++ b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss @@ -19,7 +19,9 @@ background-color: white; box-shadow: $box-shadow; border-radius: 3px; + width: max-content; min-width: 80px; + max-width: 110px; display: none; z-index: 100; From 19006d6c17f9d8fdd924a18404679516d48be17a Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 23 Mar 2026 16:20:40 +1100 Subject: [PATCH 2/8] Close action menu when making a selection But don't hide it immediately, because the user can't see if they made a selection, or accidentally closed it. Instead, fade slowly so that you can see the selected option momentarily (like system menus). This gives enough feedback while we wait for the selected action to perform. I did attempt a blink on the item background colour, like my favourite OS does which is really helpful. But couldn't get the CSS to work. --- .../vertical_ellipsis_menu_component.scss | 9 +++++++++ .../vertical_ellipsis_menu_controller.js | 16 ++++++++++++---- .../vertical_ellipsis_menu_controller_test.js | 14 +++++++++++++- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss index 75e5e6da50..965519f3b7 100644 --- a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss +++ b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_component.scss @@ -29,6 +29,15 @@ display: block; } + // Fade out so user can see which option was selected + &.selected { + transition: + opacity 0.2s linear, + visibility 0.2s linear; + opacity: 0; + visibility: hidden; + } + & > a { display: block; padding: 5px 10px; diff --git a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js index 97b13592bd..dbbe442c84 100644 --- a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js +++ b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js @@ -6,6 +6,9 @@ export default class extends Controller { connect() { super.connect(); window.addEventListener("click", this.#hideIfClickedOutside); + + // Close menu when making a selection + this.contentTarget.addEventListener("click", this.#selected.bind(this)); } disconnect() { @@ -13,17 +16,22 @@ export default class extends Controller { } toggle() { - this.contentTarget.classList.toggle("show"); + this.#toggleShow(); + } + + #selected() { + this.contentTarget.classList.add("selected"); } #hideIfClickedOutside = (event) => { if (this.element.contains(event.target)) { return; } - this.#hide(); + this.#toggleShow(false); }; - #hide() { - this.contentTarget.classList.remove("show"); + #toggleShow(force = undefined) { + this.contentTarget.classList.toggle("show", force); + this.contentTarget.classList.remove("selected"); } } diff --git a/spec/javascripts/stimulus/vertical_ellipsis_menu_controller_test.js b/spec/javascripts/stimulus/vertical_ellipsis_menu_controller_test.js index 1e8c7d1fbb..5a5956c45b 100644 --- a/spec/javascripts/stimulus/vertical_ellipsis_menu_controller_test.js +++ b/spec/javascripts/stimulus/vertical_ellipsis_menu_controller_test.js @@ -16,12 +16,13 @@ describe("VerticalEllipsisMenuController test", () => {
...
- +
`; const button = document.getElementById("button"); const content = document.getElementById("content"); + const item = document.getElementById("item"); }); it("add show class to content when toggle is called", () => { @@ -43,4 +44,15 @@ describe("VerticalEllipsisMenuController test", () => { document.body.click(); expect(content.classList.contains("show")).toBe(false); }); + + it("adds selected class to content when clicking a menu item", () => { + button.click(); + expect(content.classList.contains("selected")).toBe(false); + item.click(); + expect(content.classList.contains("selected")).toBe(true); + + // and removes it again when clicking button again + button.click(); + expect(content.classList.contains("selected")).toBe(false); + }); }); From ca3c0c98bf2564974012cd762aa23811fd703064 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 23 Mar 2026 17:22:17 +1100 Subject: [PATCH 3/8] Don't group reviewdog output Grouping is a nice feature, but it wasn't helpful here. If there's an error in rubocop for example, the rubocop section will be collapsed, and because we didn't close the group, the haml group was always open. So it wasn't clear where the error was. Better to just show all the output, which isn't very long, so you can see where the problem is straight away. Even better would be to add support for GitHub Actions annotations. I thought we used to have that turned on, not sure why it's not working now. --- script/reviewdog.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/script/reviewdog.sh b/script/reviewdog.sh index 5bde805fba..a02df06e4f 100755 --- a/script/reviewdog.sh +++ b/script/reviewdog.sh @@ -6,7 +6,7 @@ set -o pipefail -echo "::group:: Running prettier with reviewdog 🐶 ..." +echo -e "\nRunning prettier with reviewdog 🐶 ..." "$(npm root)/.bin/prettier" --check . 2>&1 | sed --regexp-extended 's/(\[warn\].*)$/\1 File is not properly formatted./' \ | reviewdog \ @@ -25,7 +25,7 @@ echo "::group:: Running prettier with reviewdog 🐶 ..." prettier=$? -echo "::group:: Running rubocop with reviewdog 🐶 ..." +echo -e "\nRunning rubocop with reviewdog 🐶 ..." bundle exec rubocop \ --fail-level info \ @@ -39,7 +39,7 @@ bundle exec rubocop \ rubocop=$? -echo "::group:: Running haml-lint with reviewdog 🐶 ..." +echo -e "\nRunning haml-lint with reviewdog 🐶 ..." bundle exec haml-lint \ --fail-level warning \ From 22a1528ac759ac524b9fa0de1241c5f0d8cc04da Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 31 Mar 2026 11:36:28 +1100 Subject: [PATCH 4/8] Show unit in tooltip Variants may have the same name, or no display_name at all. This helper method provides a more comprehensive way of describing the variant. --- app/views/admin/products_v3/_variant_row.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 139e2bef7f..73c4075f78 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -4,7 +4,7 @@ %td.col-image -# empty - variant.source_variants.each do |source_variant| - = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.display_name, source_id: source_variant.id, hub_name: variant.hub&.name)) + = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.full_name, source_id: source_variant.id, hub_name: variant.hub&.name)) %td.col-name.field.naked_inputs = f.hidden_field :id = f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name From 6013b6be7047b5a1b42bd657ba90a377e648a4fe Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 31 Mar 2026 12:07:50 +1100 Subject: [PATCH 5/8] Remove transform at end of animation During transform, any overflow on the element is clipped/hidden. This caused all dropdown menus to be clipped and unusable. Now, once the animation is complete, the overflow is visible, and menus are usable. Mistral Vibe AI was used to find this solution. I tried to find a CSS solution last week but failed, then started to consider using JS to remove the class, but decided against it once I realised that the product clone JS was already doing that, and it didn't seem to solve the clipping issue. So I asked Mistral Vibe and it suggested adding 'forwards' (before it had spent energy on evaluating the existing style rules). As you can see 'forwards' was already there, but removing it helped. So Mistral was wrong, but at least pointed me in the right direction, yay! --- app/webpacker/css/admin/products_v3.scss | 2 +- spec/system/admin/products_v3/actions_spec.rb | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index 7ea346638a..1a9fd1b755 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -375,6 +375,6 @@ .slide-in { transform-origin: top; - animation: slideInTop 0.5s forwards; + animation: slideInTop 0.5s; } } diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 897775990a..eac05fe05c 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -285,12 +285,11 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr # Close action menu (shouldn't need this, it should close itself) last_box.click - # And I can perform actions on the new product + # And I can perform actions on the new variant within last_box do page.find(".vertical-ellipsis-menu").click expect(page).to have_link "Edit" - # expect(page).to have_link "Clone" # tofix: menu is partially obscured - # expect(page).to have_link "Delete" # it's not a proper link + expect(page).to have_selector "a", text: "Delete" # it's not a proper link fill_in "Name", with: "My copy of Apples" end From 6048fcb0530f38059b6b1f09bc58ec9f93e15eeb Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Apr 2026 11:08:54 +1100 Subject: [PATCH 6/8] Define function as member arrow function This way it behaves as an instance method, and we don't have to pass in the object. --- .../vertical_ellipsis_menu_controller.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js index dbbe442c84..4d47a9683c 100644 --- a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js +++ b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js @@ -8,7 +8,7 @@ export default class extends Controller { window.addEventListener("click", this.#hideIfClickedOutside); // Close menu when making a selection - this.contentTarget.addEventListener("click", this.#selected.bind(this)); + this.contentTarget.addEventListener("click", this.#selected); } disconnect() { @@ -19,7 +19,7 @@ export default class extends Controller { this.#toggleShow(); } - #selected() { + #selected = () => { this.contentTarget.classList.add("selected"); } From cacb62f58c32ac045c50daa79a99ff946a2304a7 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Apr 2026 12:31:48 +1100 Subject: [PATCH 7/8] Style fix --- .../vertical_ellipsis_menu_controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js index 4d47a9683c..fd02f94cf9 100644 --- a/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js +++ b/app/components/vertical_ellipsis_menu_component/vertical_ellipsis_menu_controller.js @@ -21,7 +21,7 @@ export default class extends Controller { #selected = () => { this.contentTarget.classList.add("selected"); - } + }; #hideIfClickedOutside = (event) => { if (this.element.contains(event.target)) { From 15abea51abd944966ec8fc8286a5cde9a144156b Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Apr 2026 14:48:18 +1100 Subject: [PATCH 8/8] Avoid unnecessary extra page visit The second spec example below has to load the page after creating a record, so it's not helpful to load it here. --- spec/system/admin/products_v3/actions_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index eac05fe05c..0c38fd3542 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -328,12 +328,10 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr } describe "Actions columns (delete)" do - before do - visit admin_products_url - end - it "shows an actions menu with a delete link when clicking on icon for product. " \ "doesn't show delete link for the single variant" do + visit admin_products_url + within product_selector do page.find(".vertical-ellipsis-menu").click expect(page).to have_css(delete_option_selector)