Finish the component audit: keyboard nav for the filter lists, Table for Bills and reconciliation - #165
Merged
Merged
Conversation
…d nav Both built their options from raw `<button>`s inside a `PopoverContent`, so each option cost a Tab stop and the arrow keys did nothing. The Type filter is a plain single-select list, so it becomes a `DropdownMenu` with a `DropdownMenuRadioGroup` — roving focus, typeahead, `role="menuitem"` and Esc, all from the component. A radio group needs a value for every row and `null` is not one, so the "All types" row carries an `ALL_TYPES` sentinel that maps back to null at the boundary. The date presets could not follow it there. They share their popover with the custom-range From/To inputs, and Base UI's menu typeahead (`floating-ui-react/hooks/useTypeahead`) has no guard for typeable targets — inside a menu, typing a date would move the menu selection instead of filling the field. They become a vertical `ToggleGroup` instead, which is also a roving-focus composite: one Tab stop, arrow keys, Home/End, and the inputs keep working. No typeahead there, which is the honest trade. Nothing moves visually. Both lists keep `h-8`, `px-2` and the same hover fill.
…nent Both hand-rolled `<table>` and re-added, by hand, the `overflow-x-auto` container `ui/table.tsx` already ships — the same scroll wrapper written twice, in two slightly different ways. Rows sit a little tighter: `Table` pads cells `p-2` where these files set `px-3` and `px-4`. Reconciliation keeps its `px-4` because its four columns have room for it; Bills takes the component's spacing. Both keep their `min-w-[560px]`, so the mobile fix from the responsive pass still holds — the scrolling just happens in Table's container now. `widgets/account-balances.tsx` is deliberately left alone. Its `colgroup`, `table-fixed` and per-group `tbody` came from #161 and are doing work Table does not help with; converting it would put the #159 clipping fix at risk for no gain.
KenTaniguchi-R
added a commit
that referenced
this pull request
Sep 7, 2026
Base UI's composite drives arrow-key movement from `orientation`, but our wrapper destructured the prop and spent it on `data-orientation` and the class list only, so the primitive never saw it and kept its horizontal default. The vertical date-preset group added in #165 was the first caller to notice: visually a column, but Left/Right moved through it and Up/Down did nothing. ArrowDown → focus did not move ArrowRight → focus moved The four pre-existing callers are horizontal, which is why this survived until a vertical group existed. Comes with a regression test, which needs vitest to accept `.tsx` at all: `include` now covers `src/**/*.test.tsx`, and a component test declares `@vitest-environment jsdom` in its own docblock so the node default still applies to everything else. No projects, no CI change. Worth stating why the sibling wrappers are not the same bug: `size` on Card, Avatar, Select, Switch and AlertDialog, and `align` on InputGroup, are styling-only props shadcn expresses as data attributes on purpose. `orientation` is the one that changes what the primitive does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last two items from the audit that shipped in #162, reviewed as a before/after mock first.
Correction to what #162's description claimed
That PR said five popovers hand-roll their option lists and three files hand-roll
<table>. Re-reading the bodies rather than the greps, it is two and two:report-filter-barpopovers and the Account and Category filters intransaction-filtersalready useCommandwithCommandItem, which brings arrow keys and typeahead with it.category-pill.tsx:121is aPopoverTrigger render={<button>}— correct Base UI, not a hand-rolled list.widgets/account-balances.tsxwas rebuilt in feat(dashboard): make balances, net worth and transfers legible at a glance #161 withcolgroup,table-fixedand a per-grouptbodyfor subtotals. It stays as it is; converting it would put the fix(dashboard): stop Account Balances and Investments from clipping their own numbers #159 clipping fix at risk for no gain.Keyboard navigation (2 lists)
Each option was its own Tab stop and the arrow keys did nothing.
Type filter →
DropdownMenu+DropdownMenuRadioGroup. It is a plain single-select list, so the menu fits exactly: roving focus, typeahead,role="menuitem"and Esc all come from the component. A radio group needs a value for every row andnullis not one, so "All types" carries anALL_TYPESsentinel that maps back to null at the boundary.Date presets → vertical
ToggleGroup, notDropdownMenu. They share their popover with the custom-range From/To inputs, and Base UI's menu typeahead (floating-ui-react/hooks/useTypeahead) has no guard for typeable targets — inside a menu, typing a date would move the menu selection instead of filling the field.ToggleGroupis also a roving-focus composite, so the presets get one Tab stop, arrow keys and Home/End while the inputs keep working.That is one step short of what the mock showed for this list: no typeahead, and Esc closes the popover rather than the menu. Worth naming, since the mock promised the fuller behaviour for both.
Nothing moves visually — both lists keep
h-8,px-2and the same hover fill.Table (2 files)
bill-list.tsxandportfolio-reconciliation.tsxboth hand-rolled<table>and re-added theoverflow-x-autocontainerui/table.tsxalready ships — the same wrapper written twice, in two slightly different ways.bill-row.tsxcomes along asTableRow/TableCell.Rows sit slightly tighter:
Tablepads cellsp-2where these setpx-3/px-4. Reconciliation keepspx-4(four columns, room for it); Bills takes the component's spacing. Both keepmin-w-[560px], so the mobile fix still holds — the scrolling just happens in Table's container now.Testing
pnpm typecheckandpnpm lintpass.Vitest is node-only here, so component behaviour was checked with a throwaway jsdom harness: the reconciliation table renders through
Tablewith all rows, the "not itemized" badge and a totals footer summing to$1,500.00, and the date popover trigger renders. The harness is not included — adding a jsdom project to vitest is a separate call.Not yet verified interactively. The keyboard behaviour is the point of the first commit and I could not drive Chrome this session (remote debugging is off). Worth a pass through the Transactions filter bar and the Reports date picker before this is trusted.
🤖 Generated with Claude Code