apache / apache/fineract-backoffice-ui
Introduce a shared form-field component — 627 hand-copied label blocks, and the visible label is not associated with its input
- Dominant language
- TypeScript
- Stars
- 15
- Forks
- 60
- Avg merge
- 10h 15m
- Merged PRs (30d)
- 108
Description
## Business value
Every form control in the app is written out by hand in the same shape:
```html
{{ 'COMMON.NAME' | translate }}
```
Measured across `src/app`:
| | |
|---|---|
| `ion-label position="stacked"` | **627** |
| `[attr.aria-label]` on controls | **633** |
| `ion-input` using Ionic 8's native `label=` | **0** |
| Components re-declaring `.form-container { padding: 24px; max-width: N; margin: 0 auto }` | **87**, in **6** different max-widths |
Three separate costs.
**1. The visible label does not label the input.** In Ionic 8 the association comes from the component's own `label` property or a slotted label — `node_modules/@ionic/core/dist/collection/components/input/input.js:467-476`:
```js
getLabelledById() {
if (this.inheritedAttributes['aria-label']) { return undefined; }
if (this.label !== undefined) { return this.labelTextId; }
return this.labelSlot?.id || undefined;
}
```
A sibling `` sitting next to the input inside an `` is neither. It is the Ionic 7 pattern, and `STYLE.md:76` still prescribes it. So the visible text is decorative, and the accessible name comes **only** from the `[attr.aria-label]` — which is why all 633 of them exist. It works today, which is exactly why nobody has noticed that 627 of them are a workaround for markup that is one major version out of date. And it is fragile in a specific way: delete an `aria-label` as redundant-looking and the field silently becomes unnamed.
**2. Roughly 4,000 lines of duplicated template.** 555 non-date control blocks average ~14 lines each; 73 date-picker triads (`ion-datetime-button` + `ion-modal` + `ion-datetime`) average ~18. The two largest forms are ~65% template by line count, and that template is almost entirely field markup.
**3. Nowhere to put a validation message.** There is essentially no inline validation feedback anywhere in the app — one hand-rolled cross-field message in `journal-entry-form.component.ts:283-285`, and nothing else. Server-side errors go to a global toast that names the parameter in backend terms (`error.interceptor.ts:63-77`). A field-level error has no home to go to, and adding one today means editing hundreds of blocks.
## Why this is worth doing before any forms rework
There is a live question about whether to move off template-driven forms. **This change is worth doing either way, and it makes whichever answer you pick dramatically cheaper.** It is the load-bearing step: once field markup lives in one component, swapping what drives it is an internal change rather than a 128-file rewrite.
For context on that separate question — `@angular/forms@22.0.7` already ships `./signals` and `./signals/compat` as stable public API. `form(model: WritableSignal, schema)` takes a writable signal holding a plain object, which is exactly the shape this codebase already uses (`readonly product = signal({...})`), and `validateHttp` maps an HTTP error response onto field errors. That is a strong argument against migrating to reactive forms as an intermediate step. **But that decision is not this issue** — do not make it here.
## Describing the change
Two or three components in `src/app/shared/components/`:
- `` — label, control projection, required marker, hint, error slot
- `` — same, wrapping `ion-select`
- `` — the `ion-datetime-button` + `ion-modal` + `ion-datetime` triad, which is the most-copied and most error-prone block
Use Ionic 8's native labelling — `label="…"` and `labelPlacement="stacked"` on the control itself — so the association is real and the `aria-label` becomes unnecessary rather than load-bearing.
Signal inputs, per the house style (73 `input()`, one `@Input()` left in the codebase). Take the label as a translation key and translate inside, matching how `ColumnDef.label` already works.
**This is deliberately not a full migration.** Land the components plus **one** converted form as the proof, and stop. Pick a mid-sized form — `src/app/features/organization/offices/office-form.component.ts` or similar — not `loan-product-form.component.ts`. Converting 150 forms in one PR is unreviewable and will conflict with everything.
**Four things to get right.**
1. **Do not break `[(ngModel)]`.** 574 bindings depend on it, and the wrapper must project the control rather than own it, or every call site changes shape. Content projection with the consumer supplying the `ion-input` is the safe design; a wrapper that renders its own input and re-exposes a value binding is a much larger change and should not be attempted here.
2. **Keep `name` on the control.** Template-driven forms register by `name`; a wrapper that swallows it silently breaks form validity, and `[disabled]="form.invalid"` on 92 submit buttons then never enables.
3. **Verify the accessible name after converting.** Confirm the control's accessible name is the label — via `getByLabel()` in a Playwright check, or `getByRole('textbox', { name: … })`. This is the whole point of using the native property; assert it rather than assuming.
4. **Fold the `.form-container` styles into `src/styles/_common.scss`** as a shared class while you are here. 87 copies in 6 max-width variants is a visual inconsistency as much as a duplication, and it is a two-line change per file afterwards.
## Testing
No platform acceptance test applies — this is presentation, and the platform contract is unchanged. That is the property the tests should pin down.
- **A unit spec** for each new component: label renders, translation key resolves, required marker appears, the projected control keeps its `name`, and the accessible name equals the label.
- **The converted form's existing spec must pass untouched.** If you find yourself editing its assertions, the wrapper has changed behaviour and the design needs revisiting. This is the strongest signal available that the refactor is faithful — say in the PR whether you had to touch it.
- **An e2e assertion using `getByLabel()`** on the converted form. The suite currently has **zero** `getByLabel` calls, which is itself a symptom: labels are not associated, so nobody could write one. A passing `getByLabel` is direct proof the association now works.
## Scope
In scope: the shared components, the shared container styles, and one converted form.
Out of scope: converting the rest; any change to how form state is held; validation-message rendering beyond leaving a slot for it; the choice between template-driven, reactive and signal forms.
## Getting started
- Existing shared components for house style: `src/app/shared/components/data-table/`, `src/app/shared/components/status-badge/`
- Style guidance: `STYLE.md` — note it prescribes the Ionic 7 label pattern this issue is moving away from. **Update it in the same PR**, or the next contributor will reintroduce the old shape.
- Adapter boundary: `DOCS/adr/0003-adapter-boundary.md`. Use `| appTranslate` from `src/app/core/adapters` in new components; importing `@ngx-translate/core` directly is banned by lint outside the composition root.
- `npm test`, `npm run lint:prune`, `npm run i18n:check` and `npm run build` must pass.
- Worth agreeing the component API on the issue before writing much — it will end up at hundreds of call sites, so the input names are worth ten minutes of discussion.
Contributor guide
Research direction
Start with the house-style components in src/app/shared/components/data-table/ and status-badge/, then read the adapter guidance in DOCS/adr/0003-adapter-boundary.md and STYLE.md. Examine office-form.component.ts, src/styles/_common.scss, and its existing spec before agreeing the shared component API. Done means the new components and one converted form pass the stated unit, e2e, lint, translation, test, and build checks, with the accessible name verified by getByLabel().
Written by the indexing model from the issue text.
Assessment
- Tech stack
- angular, playwright, typescript
- Domain
- accessibility, documentation, frontend, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100