Files
sure/app/views/holdings/_cost_basis_cell.html.erb
Guillem Arias Fauste 12785754c8 feat(design-system): split DS::Menu into strict action-list + new DS::Popover (#1850)
* feat(design-system): split DS::Menu into strict action-list + new DS::Popover for mixed content

Closes #1743.

DS::Menu used to absorb both action-list dropdowns (row context menus,
"more actions") AND mixed-content panels (user-account dropdown,
filter forms, picker pop-ups). The two shapes carry incompatible a11y
contracts:

- **Action list**: `role="menu"` container, `role="menuitem"` children,
  Up/Down arrow nav per WAI-ARIA APG.
- **Mixed content**: NO menu role — `role="menu"` restricts AT users
  to menuitem-only navigation and breaks any panel with forms,
  headings, or generic groupings.

This PR splits the component:

## DS::Menu (tightened)

Strict action-list primitive. Variants reduced to `:icon` and
`:button` (no `:avatar`). `custom_content` slot removed. Bakes in:

- `role="menu"` on the panel, `aria-haspopup="menu"` +
  `aria-expanded` + `aria-controls` on the trigger.
- `role="menuitem"` + `tabindex="-1"` on every DS::MenuItem; the
  controller installs roving tabindex (first item gets `tabindex="0"`
  when the menu opens) and handles ArrowUp/Down/Home/End +
  Escape + Enter/Space activation.
- `role="separator"` on the divider variant.
- Stable per-instance `menu-<8-char hex>` id so the trigger's
  `aria-controls` resolves correctly.

`DS::Menu.new(variant: :avatar, ...)` now raises ArgumentError
pointing at DS::Popover.

## DS::Popover (new)

Positioned panel for **mixed**, **non-action-list** content: account
menus, picker forms, filter forms, embedded controls. Slots: `button`,
`header`, `custom_content`. Variants: `:icon`, `:button`, `:avatar`.
NO `role="menu"` — the panel announces as a generic dialog-popup
(`aria-haspopup="dialog"`, `aria-expanded`, `aria-controls`).
Mirrors DS::Menu's floating-ui positioning + Escape/outside-click
lifecycle in its own Stimulus controller (`DS--popover`). Avatar
variant ships a focus ring + bumped touch target (44×44 via `w-11
h-11` per #1738).

## Migrated callsites (7 → DS::Popover)

- `app/views/users/_user_menu.html.erb` — avatar trigger + profile
  header + nav links (items kept as DS::MenuItem inside
  `custom_content` for visual parity)
- `app/views/categories/_menu.html.erb` — turbo-framed category picker
- `app/views/budgets/_budget_header.html.erb` — budget picker
- `app/views/reports/index.html.erb` — period picker
- `app/views/holdings/_cost_basis_cell.html.erb` — cost-basis edit form
- `app/views/transactions/searches/_form.html.erb` — filter form
- `app/components/UI/account/activity_feed.html.erb:70` — status
  checkboxes (the row-level "new" menu on line 9 stays as DS::Menu)

The other 33 DS::Menu callsites stay as-is — pure action lists.

Locale: `ds.popover.avatar_default_label` + `users.user_menu.aria_label`
keys added (en only; other locales handled in a separate i18n pass).

* fix(test): update sidebar user-menu selector for Menu→Popover migration

The user-menu now renders as `DS::Popover` (variant: :avatar) instead
of `DS::Menu` after the menu split, so its trigger carries
`data-DS--popover-target="button"` rather than the old
`data-DS--menu-target`. Update the sidebar-driven settings test helper
to match — every system test that drives Settings via the sidebar
gates on this selector.

* fix(review): DS::Popover/Menu trigger a11y + caller-attr preservation

- popover.rb / menu.rb: button slot now merges (not overwrites) caller-
  provided data and aria hashes, sets aria-haspopup/expanded/controls on
  the :button variant, defaults type="button" on block-rendered buttons.
- menu.rb / menu.html.erb: drop renders_one :header (strict-menu API
  shouldn't expose an arbitrary-markup escape hatch); preview updated.
- menu_controller.js: handle Enter/Space activation on focused menuitem
  so keyboard navigation matches the ARIA menu pattern.
- cost_basis_cell / transactions/searches/_menu: retarget cancel button
  data-action from DS--menu#close to DS--popover#close (host controller
  changed in the migration).

* fix: apply CodeRabbit auto-fixes

Fixed 1 file(s) based on 1 unresolved review comment.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>

* fix(review): MenuItem roving: false for DS::Popover usage

Codex P1 on #1850: \`DS::MenuItem\` hard-codes \`tabindex=\"-1\"\` and
\`role=\"menuitem\"\` for both link and button variants — correct
inside \`DS::Menu\` (which provides arrow-key roving and announces
\`role=\"menu\"\`), but breaks every \`DS::MenuItem\` rendered inside
\`DS::Popover\` (\`app/views/users/_user_menu.html.erb\`). Popover has
no roving handler, so Tab skips every item — Settings, Changelog,
Feedback, Contact, Log out become keyboard-unreachable.

Add a \`roving:\` keyword (default \`true\`) to \`DS::MenuItem\` that
gates both \`tabindex=\"-1\"\` and \`role=\"menuitem\"\`. \`DS::Menu\`
callers keep the default (roving menu semantics intact). Pass
\`roving: false\` from \`_user_menu.html.erb\` so user-menu items land
in the normal Tab order. Existing \`menu.with_item(...)\` callers in
the design system still default to \`true\`, so no behavior change for
\`DS::Menu\` consumers.

* fix(review): make menuitem_attrs authoritative on roving

CodeRabbit Major on #1850: \`merged_opts\` was splatted AFTER
\`menuitem_attrs\` in \`DS::MenuItem#wrapper\`, so a stray
\`role: :button\` or \`tabindex: 0\` from a \`menu.with_item(..., role: …)\`
caller could silently downgrade the \`DS::Menu\` ARIA contract that
\`menuitem_attrs\` enforces.

Strip \`:role\` and \`:tabindex\` from \`merged_opts\` whenever
\`roving\` is enabled, then splat \`menuitem_attrs\` last. When
\`roving: false\` (popover usage in \`_user_menu.html.erb\`) callers
keep full control — Tab order and explicit ARIA stay tunable by the
caller.

---------

Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
2026-05-20 18:30:25 +02:00

110 lines
5.5 KiB
Plaintext

<%# locals: (holding:, editable: true) %>
<%
# Pre-calculate values for the form
# Note: cost_basis field stores per-share cost, so calculate total for display
current_per_share = holding.cost_basis.present? && holding.cost_basis.positive? ? holding.cost_basis : nil
current_total = current_per_share && holding.qty.positive? ? (current_per_share * holding.qty).round(2) : nil
currency = Money::Currency.new(holding.currency)
%>
<%= turbo_frame_tag dom_id(holding, :cost_basis) do %>
<% if holding.cost_basis_locked? && !editable %>
<%# Locked and not editable (from holdings list) - just show value, right-aligned %>
<div class="flex items-center justify-end gap-1">
<%= tag.span format_money(holding.avg_cost), class: "privacy-sensitive" %>
<%= icon "lock", size: "xs", class: "text-secondary" %>
</div>
<% else %>
<%# Unlocked OR editable context (drawer) - show clickable menu %>
<%= render DS::Popover.new(variant: :button, placement: "bottom-end") do |popover| %>
<% popover.with_button(class: "hover:text-primary cursor-pointer group") do %>
<% if holding.avg_cost %>
<div class="flex items-center gap-1">
<%= tag.span format_money(holding.avg_cost), class: "privacy-sensitive" %>
<% if holding.cost_basis_locked? %>
<%= icon "lock", size: "xs", class: "text-secondary" %>
<% end %>
<%= icon "pencil", size: "xs", class: "text-secondary opacity-0 group-hover:opacity-100 transition-opacity" %>
</div>
<% else %>
<div class="flex items-center gap-1 px-2 py-0.5 rounded text-secondary hover:text-primary hover:bg-container-inset-hover transition-colors">
<%= icon "pencil", size: "xs" %>
<span class="text-xs"><%= t(".set") %></span>
</div>
<% end %>
<% end %>
<% popover.with_custom_content do %>
<div class="p-4 min-w-[280px]"
data-controller="cost-basis-form"
data-cost-basis-form-qty-value="<%= holding.qty %>">
<h4 class="font-medium text-sm mb-3">
<%= t(".set_cost_basis_header", ticker: holding.ticker, qty: format_quantity(holding.qty)) %>
</h4>
<%
form_data = { turbo: false }
if holding.avg_cost
form_data[:turbo_confirm] = {
title: t(".overwrite_confirm_title"),
body: t(".overwrite_confirm_body", current: format_money(holding.avg_cost))
}
end
%>
<%= styled_form_with model: holding,
url: holding_path(holding),
method: :patch,
class: "space-y-3",
data: form_data do |f| %>
<!-- Primary: Total cost basis (custom input, no spinners) -->
<div class="form-field">
<div class="form-field__body">
<label class="form-field__label"><%= t(".total_cost_basis_label") %></label>
<div class="flex items-center gap-1">
<span class="text-secondary text-sm font-medium"><%= currency.symbol %></span>
<input type="number" step="any"
name="holding[cost_basis]"
class="form-field__input grow"
placeholder="0.00"
autocomplete="off"
value="<%= sprintf("%.2f", current_total) if current_total %>"
data-action="input->cost-basis-form#updatePerShare"
data-cost-basis-form-target="total">
<span class="text-secondary text-sm"><%= currency.iso_code %></span>
</div>
</div>
</div>
<p class="text-xs text-secondary -mt-2" data-cost-basis-form-target="perShareDisplay">
= <%= currency.symbol %><span data-cost-basis-form-target="perShareValue"><%= number_with_precision(current_per_share, precision: 2) || "0.00" %></span> <%= t(".per_share") %>
</p>
<!-- Alternative: Per-share input -->
<div class="pt-2 border-t border-tertiary">
<label class="text-xs text-secondary block mb-1"><%= t(".or_per_share_label") %></label>
<div class="flex items-center gap-1">
<span class="text-secondary text-sm font-medium"><%= currency.symbol %></span>
<input type="number" step="any"
class="form-field__input grow"
placeholder="0.00"
autocomplete="off"
value="<%= sprintf("%.2f", current_per_share) if current_per_share %>"
data-action="input->cost-basis-form#updateTotal"
data-cost-basis-form-target="perShare">
<span class="text-secondary text-sm"><%= currency.iso_code %></span>
</div>
</div>
<div class="flex justify-end gap-2 pt-2">
<button type="button"
class="inline-flex items-center gap-1 px-2 py-1 rounded-md text-sm font-medium text-primary button-bg-secondary-strong hover:button-bg-secondary-strong-hover"
data-action="click->DS--popover#close">
<%= t(".cancel") %>
</button>
<%= f.submit t(".save"), class: "inline-flex items-center gap-1 px-2 py-1 rounded-md text-sm font-medium text-inverse bg-inverse hover:bg-inverse-hover" %>
</div>
<% end %>
</div>
<% end %>
<% end %>
<% end %>
<% end %>