Aman-Mittal opened a new issue, #287:
URL: https://github.com/apache/fineract-backoffice-ui/issues/287
## Business value
Every form control in the app is written out by hand in the same shape:
```html
<ion-item fill="outline">
<ion-label position="stacked">{{ 'COMMON.NAME' | translate }}</ion-label>
<ion-input
[attr.aria-label]="'COMMON.NAME' | translate"
[name]="'colName' + i"
[(ngModel)]="column.name"
required
></ion-input>
</ion-item>
```
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 `<ion-label>` sitting next to the input inside an `<ion-item>` 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/[email protected]` already
ships `./signals` and `./signals/compat` as stable public API.
`form<TModel>(model: WritableSignal<TModel>, schema)` takes a writable signal
holding a plain object, which is exactly the shape this codebase already uses
(`readonly product = signal<PostLoanProductsRequest>({...})`), 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/`:
- `<app-form-field>` — label, control projection, required marker, hint,
error slot
- `<app-select-field>` — same, wrapping `ion-select`
- `<app-date-field>` — 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
