From ab175cc43ccadfff464c1199aed9cf1b63a6b09b Mon Sep 17 00:00:00 2001 From: GFR Date: Sat, 5 Sep 2026 08:27:16 +0200 Subject: [PATCH] fix(ui): make whole list row clickable, not just the name text (#3367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ui): make whole list row clickable, not just the name text Fixes #3366. Transaction, split-parent, trade, and account list rows carry hover styling that implies the whole row is clickable, but only the name text actually opened the detail drawer — clicking the avatar, amount, or whitespace between them did nothing. Add a clickable-row Stimulus controller that delegates a click anywhere on the row to its primary link, while leaving real interactive descendants (checkbox, category menu, account link, quick-edit badge, kebab menu) to handle their own clicks. Co-Authored-By: Claude Sonnet 5 * fix(ui): exclude popover/menu panels from row click delegation, preserve modifier clicks Codex review on #3367 flagged two issues in clickable_row_controller: - Category/account popovers render as position:fixed but stay DOM descendants of the row, so clicking their background (padding, headings, plain text) fell through to the row's link. - linkTarget.click() discarded Ctrl/Cmd/Shift modifiers, so modified clicks opened in the current frame instead of a new tab. * fix(ui): exclude quick-edit dropdown, use window.open for modified clicks CodeRabbit review on #3367 found two more issues: - The investment activity quick-edit dropdown is a hand-rolled absolute-positioned panel (not DS::Popover/DS::Menu), so it wasn't covered by the earlier popover/menu exclusion and background clicks inside it fell through to the row link. - Redispatching a synthetic MouseEvent with modifier flags doesn't actually open a new tab: browsers only honor Ctrl/Cmd/Shift on trusted, native click events, so the previous "fix" was cosmetic. Explicitly call window.open() for modified clicks instead. * fix(ui): make Dividend/Interest quick-edit badge not swallow row clicks jjmata found that the activity-label badge renders as a real + <% end %> <% unless income_trade %> diff --git a/app/views/trades/_trade.html.erb b/app/views/trades/_trade.html.erb index 162a7da09..95ac57d86 100644 --- a/app/views/trades/_trade.html.erb +++ b/app/views/trades/_trade.html.erb @@ -5,7 +5,9 @@ <%= turbo_frame_tag dom_id(entry) do %> <%= turbo_frame_tag dom_id(trade) do %> -
text-sm font-medium p-4"> +
text-sm font-medium p-4" + data-controller="clickable-row" + data-action="click->clickable-row#open">
<%= check_box_tag dom_id(entry, "selection"), class: "checkbox checkbox--light hidden lg:block", @@ -41,7 +43,7 @@
<%= link_to entry.name, entry_path(entry), - data: { turbo_frame: "drawer", turbo_prefetch: false }, + data: { turbo_frame: "drawer", turbo_prefetch: false, clickable_row_target: "link" }, class: "hover:underline" %>
<% end %> diff --git a/app/views/transactions/_split_parent_row.html.erb b/app/views/transactions/_split_parent_row.html.erb index f2e68eca6..e6227351f 100644 --- a/app/views/transactions/_split_parent_row.html.erb +++ b/app/views/transactions/_split_parent_row.html.erb @@ -1,7 +1,9 @@ <%# locals: (entry:) %> <% transaction = entry.entryable %> -
+
<%# Empty space where checkbox would be, for alignment %> @@ -31,7 +33,7 @@
<%= link_to entry.name, entry_path(entry), - data: { turbo_frame: "drawer", turbo_prefetch: false }, + data: { turbo_frame: "drawer", turbo_prefetch: false, clickable_row_target: "link" }, class: "hover:underline" %>
diff --git a/app/views/transactions/_transaction.html.erb b/app/views/transactions/_transaction.html.erb index 213e11964..98adcf26e 100644 --- a/app/views/transactions/_transaction.html.erb +++ b/app/views/transactions/_transaction.html.erb @@ -2,10 +2,17 @@ <% transaction = entry.entryable %> <% transaction_security_logo_url = transaction.activity_security&.display_logo_url %> +<% open_path = if transaction.transfer? + transaction.transfer.present? ? transfer_path(transaction.transfer) : entry_path(entry) + else + in_split_group ? entry_path(entry, grouped: true) : entry_path(entry) + end %> <%= turbo_frame_tag dom_id(entry) do %> <%= turbo_frame_tag dom_id(transaction) do %> -
"> +
" + data-controller="clickable-row" + data-action="click->clickable-row#open">
<%= check_box_tag dom_id(entry, "selection"), @@ -66,27 +73,16 @@
- <% if transaction.transfer? %> - <%= link_to( - entry.name, - transaction.transfer.present? ? transfer_path(transaction.transfer) : entry_path(entry), - data: { - turbo_frame: "drawer", - turbo_prefetch: false - }, - class: "hover:underline" - ) %> - <% else %> - <%= link_to( - entry.name, - in_split_group ? entry_path(entry, grouped: true) : entry_path(entry), - data: { - turbo_frame: "drawer", - turbo_prefetch: false - }, - class: "hover:underline" - ) %> - <% end %> + <%= link_to( + entry.name, + open_path, + data: { + turbo_frame: "drawer", + turbo_prefetch: false, + clickable_row_target: "link" + }, + class: "hover:underline" + ) %>
diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 2bb01af1b..44fa65cc5 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -16,6 +16,18 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest assert_select "p.ml-auto.privacy-sensitive" end + test "index delegates whole-row account clicks to the account link" do + get accounts_url + + assert_response :success + doc = Nokogiri::HTML::Document.parse(response.body) + row = doc.at_css("turbo-frame##{dom_id(@account)} [data-controller='clickable-row']") + account_link = row.at_css("a[data-clickable-row-target='link']") + + assert_equal "click->clickable-row#open", row["data-action"] + assert_equal account_path(@account), account_link["href"] + end + test "index localizes the Plaid add accounts action" do ensure_tailwind_build @user.update!(locale: "de") @@ -128,6 +140,21 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest assert_equal 0, per_row_transfer, "N+1 per-row transfer queries detected (#{per_row_transfer})" end + test "show delegates whole-row trade clicks to the drawer link" do + investment_account = accounts(:investment) + entry = entries(:trade) + + get account_url(investment_account) + + assert_response :success + doc = Nokogiri::HTML::Document.parse(response.body) + row = doc.at_css("turbo-frame##{dom_id(entry.entryable)} [data-controller='clickable-row']") + drawer_link = row.at_css("a[data-clickable-row-target='link']") + + assert_equal "click->clickable-row#open", row["data-action"] + assert_equal entry_path(entry), drawer_link["href"] + end + test "show avoids N+1 split-parent queries across paginated entries" do queries = capture_sql_queries { get account_url(@account) } assert_response :success diff --git a/test/controllers/transactions_controller_test.rb b/test/controllers/transactions_controller_test.rb index 570ea1c65..4dcfe010c 100644 --- a/test/controllers/transactions_controller_test.rb +++ b/test/controllers/transactions_controller_test.rb @@ -488,6 +488,43 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest assert_select ".split-group > div.opacity-50 p.privacy-sensitive", count: 1 end + # Row only opened on a precise click on the name text (whitespace between + # name/avatar/amount looked clickable via the row's hover styling but did + # nothing). A row-level click delegates to the name link now, so the whole + # row opens the drawer while interactive descendants (checkbox, category + # menu, account link) keep handling their own clicks. + test "transaction row delegates whole-row clicks to the drawer link" do + get transactions_url + + assert_response :success + doc = Nokogiri::HTML::Document.parse(response.body) + frame_id = ActionView::RecordIdentifier.dom_id(@entry.entryable) + row = doc.at_css("turbo-frame##{frame_id} [data-controller='clickable-row']") + drawer_link = row.at_css("a[data-clickable-row-target='link']") + + assert_equal "click->clickable-row#open", row["data-action"] + assert_equal entry_path(@entry), drawer_link["href"] + end + + test "split parent row delegates whole-row clicks to the drawer link" do + entry = create_transaction(account: accounts(:depository), amount: 100, name: "Split parent") + + entry.split!([ + { name: "Part 1", amount: 60, category_id: nil }, + { name: "Part 2", amount: 40, category_id: nil } + ]) + + get transactions_url + + assert_response :success + doc = Nokogiri::HTML::Document.parse(response.body) + row = doc.at_css(".split-group [data-controller='clickable-row']") + drawer_link = row.at_css("a[data-clickable-row-target='link']") + + assert_equal "click->clickable-row#open", row["data-action"] + assert_equal entry_path(entry), drawer_link["href"] + end + test "can paginate" do family = families(:empty) sign_in users(:empty)