|
| 1 | +# P1 spike — PrimeReact DataTable + wrapper migration findings |
| 2 | + |
| 3 | +Branch: `feat/primereact-migration` · primereact@10.9.7 |
| 4 | + |
| 5 | +Consolidated P1 findings so P3 (call-site migration) and P4 (live-browser |
| 6 | +verification) are de-risked. Findings below are from **reading code + the |
| 7 | +PrimeReact `.d.ts` type defs** — no live browser was available, so anything |
| 8 | +that depends on rendered layout/interaction is explicitly flagged for P4. |
| 9 | + |
| 10 | +--- |
| 11 | + |
| 12 | +## 1. DataTable responsive strategy: `stack` vs `scrollable` |
| 13 | + |
| 14 | +`src/components/ui/pr/data-table.tsx` uses: |
| 15 | + |
| 16 | +```tsx |
| 17 | +<DataTable responsiveLayout="stack" breakpoint={responsiveBreakpoint} … /> |
| 18 | +``` |
| 19 | + |
| 20 | +Type-def findings (`node_modules/primereact/datatable/datatable.d.ts`): |
| 21 | + |
| 22 | +- `responsiveLayout?: 'scroll' | 'stack'` — **line 1421, marked |
| 23 | + `@deprecated since version 9.2.0`**. It is still present and functional in |
| 24 | + 10.9.7 (not removed), so the wrapper keeps it per spec, with a code comment |
| 25 | + noting the deprecation. **Risk: a future PrimeReact major may remove it.** |
| 26 | +- `breakpoint?: string` (line 1127) — current, used by stack layout as the |
| 27 | + max-width boundary below which rows stack into label/value cards. |
| 28 | +- `scrollable?: boolean` (line 1466) — the non-deprecated path; pairs with |
| 29 | + `scrollHeight` / frozen columns for wide tables. |
| 30 | + |
| 31 | +Recommendation for the two table families: |
| 32 | + |
| 33 | +| Table | Today | Recommended PR approach | |
| 34 | +| :-- | :-- | :-- | |
| 35 | +| `transactions-table` (user-facing, ~7 cols) | wrapper div `overflow-x-auto` at <lg, sticky thead | `responsiveLayout="stack"` is a reasonable fit — narrow viewport stacks each txn into a card. | |
| 36 | +| `members-directory-table` (5 cols + expansion) | `overflow-x-auto` at <lg | `stack` fits; expansion still works in stack mode (see §3). | |
| 37 | +| **Wide ADMIN tables** (pending, bank-accounts, loan detail) | horizontal scroll | Likely want **`scrollable`** instead of `stack` — stacking 8–10 admin columns into cards is unwieldy. Decide per-table in P3. | |
| 38 | + |
| 39 | +**⚠️ P4 (live browser) must confirm:** that stacked cards are readable on a |
| 40 | +real narrow viewport, that the deprecated `responsiveLayout="stack"` still |
| 41 | +renders cards (not just a scroll) in 10.9.7's runtime, and that sticky-header |
| 42 | +behaviour is acceptable vs the current hand-rolled `.sticky-thead`. If stack |
| 43 | +proves wrong for admin tables, swap those call sites to `scrollable`. |
| 44 | + |
| 45 | +--- |
| 46 | + |
| 47 | +## 2. Export compatibility (jspdf / CSV) — **SURVIVES the migration** |
| 48 | + |
| 49 | +Code-based finding (definitive): the export is **data-array driven, NOT |
| 50 | +DOM-scraping.** It is library-agnostic and survives the table swap untouched. |
| 51 | + |
| 52 | +- `src/components/table-export.tsx` (`TableExportMenu`) receives `columns`, |
| 53 | + `rows: Cell[][]`, `footer`, `criteria` as **props** and forwards them to |
| 54 | + `exportToCsv` / `exportToPdf`. |
| 55 | +- `src/lib/table-export.ts`: |
| 56 | + - `exportToCsv` stringifies the in-memory `rows` matrix directly. |
| 57 | + - `exportToPdf` lazy-imports jspdf + jspdf-autotable and calls |
| 58 | + `autoTable(doc, { head: [columns], body: rows.map(...), foot: ... })` — |
| 59 | + it reads the **passed-in arrays**, never `document` / no DOM table is |
| 60 | + scraped. (The only `document` use is `triggerDownload` creating an `<a>`.) |
| 61 | +- The caller (`transactions-table.tsx`) builds `exportColumns` / `exportRows` |
| 62 | + / `exportFooter` from the same `sorted` array it renders — so export |
| 63 | + reflects current sort + search filter. |
| 64 | + |
| 65 | +**P3 implication:** when migrating a table to `PrDataTable`, keep building the |
| 66 | +`exportRows`/`exportColumns` arrays from the same source data and keep mounting |
| 67 | +`<TableExportMenu>` above the table. The `PrDataTable` wrapper deliberately |
| 68 | +does NOT own export — leave that wiring in the call site. No jspdf changes |
| 69 | +needed. If P3 moves sorting/filtering into the DataTable's own state, just |
| 70 | +ensure the export arrays are derived from the same sorted/filtered data so the |
| 71 | +"export = what's on screen" contract is preserved. |
| 72 | + |
| 73 | +--- |
| 74 | + |
| 75 | +## 3. Expandable rows |
| 76 | + |
| 77 | +Today (`members-directory-table.tsx`): hand-rolled. |
| 78 | + |
| 79 | +- Local `expanded: Set<string>` state, `toggle(id)` adds/removes. |
| 80 | +- Each member renders as a `<Fragment>` with the main `<tr>` plus, when |
| 81 | + `expanded.has(m.id)`, a second `<tr>` whose single `<td colSpan={5}>` hosts |
| 82 | + `<MemberDetailPanel>`. |
| 83 | +- The toggle control is a custom `<ExpandToggle>` in the last cell. |
| 84 | +- Expansion is per-row independent (a Set, not single-row). |
| 85 | + |
| 86 | +PrimeReact `rowExpansionTemplate` (wrapper exposes `rowExpansion?: (row) => ReactNode`): |
| 87 | + |
| 88 | +- DataTable manages expansion via `expandedRows` (line 1244: |
| 89 | + `DataTableValueArray | DataTableExpandedRows`) + `onRowToggle`, and renders |
| 90 | + `rowExpansionTemplate(data, options)` (line 1779) below the row. |
| 91 | +- A toggle column is added by giving a `<Column expander />` (the wrapper's |
| 92 | + current `PrColumn` type does NOT expose `expander` — see gap below). |
| 93 | + |
| 94 | +**Call-site migration work for P3:** |
| 95 | + |
| 96 | +1. The wrapper passes `rowExpansionTemplate` but does **not** currently wire |
| 97 | + `expandedRows` / `onRowToggle`, and `PrColumn` has no `expander` flag. So |
| 98 | + as written, `rowExpansion` alone won't render an expander toggle or track |
| 99 | + open state. **P3 must extend the wrapper** to either (a) own internal |
| 100 | + `expandedRows` state + auto-inject an expander column, or (b) accept |
| 101 | + `expandedRows`/`onRowToggle` props from the call site. Pick one in P3. |
| 102 | +2. The current Set-based multi-expand maps cleanly to PrimeReact's |
| 103 | + `DataTableExpandedRows` object (keyed by `dataKey`) — multi-row expand is |
| 104 | + supported, no behaviour loss. |
| 105 | +3. `MemberDetailPanel` and its child forms (`AddContactForm`, |
| 106 | + `BankAccountForm`, `MemberBankAccountsManager`) move into the |
| 107 | + `rowExpansion={(row) => <MemberDetailPanel … />}` callback as-is. |
| 108 | +4. `MembersDirectoryTable` splits into Active / Inactive accordions and mounts |
| 109 | + `<TableExportMenu>` separately — that structure stays; only the inner |
| 110 | + `<table>` per section becomes a `PrDataTable`. |
| 111 | + |
| 112 | +**⚠️ P4 must confirm** expand/collapse animation and that stacked (mobile) |
| 113 | +mode still exposes the expander. |
| 114 | + |
| 115 | +--- |
| 116 | + |
| 117 | +## 4. Already-discovered P1 contract gaps for the other wrappers |
| 118 | + |
| 119 | +Recorded here so all P1 migration risks live in one place. These concern the |
| 120 | +sibling wrappers in `src/components/ui/pr/`. |
| 121 | + |
| 122 | +### Selects — `dropdown.tsx` / `multiselect.tsx` |
| 123 | + |
| 124 | +- **Existing** `searchable-select` / multi-select: options shaped as |
| 125 | + `{ id, name }`, integrate via **form-post hidden inputs** (FormData), and |
| 126 | + support `emptyOption` / select-all semantics. |
| 127 | +- **New** `PrDropdown` / `PrMultiSelect`: `{ value, label }` options, |
| 128 | + **controlled value** (no hidden input, no FormData wiring). |
| 129 | +- **P3 work:** add an option-mapping adapter (`{id,name}` → `{value,label}`) |
| 130 | + and **lift selection to controlled React state** at each call site; either |
| 131 | + re-add a hidden `<input name>` for actions that read FormData, or switch |
| 132 | + those actions to read from controlled state. Re-implement |
| 133 | + emptyOption/select-all if the call site needs them. |
| 134 | + |
| 135 | +### Amount input — `amount-input.tsx` |
| 136 | + |
| 137 | +- **Existing** `AmountInput`: **UNCONTROLLED**, ships a hidden `<input name>` |
| 138 | + so FormData picks up the value, has a `showWords` (amount-in-words) feature |
| 139 | + and a ₹ prefix. |
| 140 | +- **New** `PrAmountInput`: **CONTROLLED** `number | null`, no FormData wiring. |
| 141 | +- **P3 work:** lift amount to controlled state (or add a hidden input mirroring |
| 142 | + the controlled value) so server actions still receive it via FormData; |
| 143 | + re-implement `showWords` if the form still needs the amount-in-words helper. |
| 144 | + |
| 145 | +--- |
| 146 | + |
| 147 | +## Summary of residual risks for P3/P4 |
| 148 | + |
| 149 | +- `responsiveLayout` is deprecated-but-functional → P4 confirm in browser; |
| 150 | + consider `scrollable` for wide admin tables. |
| 151 | +- Wrapper needs expansion-state + expander-column wiring before |
| 152 | + members-directory can migrate (§3.1). |
| 153 | +- Selects + amount input need controlled-state / FormData adapters (§4). |
| 154 | +- Export is safe — no work needed beyond keeping array derivation in sync (§2). |
0 commit comments