Aman-Mittal opened a new pull request, #848:
URL: https://github.com/apache/fineract-backoffice-ui/pull/848
Closes the first part of #287 — the component plus **one** converted form,
which is the scope that issue asks for. Everything else it lists stays out
deliberately, itemised at the bottom.
## The defect, not just the duplication
Every form here writes the same block by hand: an `ion-item`, an `ion-label
position="stacked"` *beside* the control, an `[attr.aria-label]` repeating the
same string, and ten more lines for the error. 627 of the first, 633 of the
second.
In Ionic 8 that sibling label names nothing. `getLabelledById()` in
`@ionic/core` reads the control's own `label` property or its `label` slot and
nothing else, so the accessible name came **only** from the hand-repeated
`aria-label`. It works — which is why nobody noticed that 627 labels are a
workaround for markup one major version out of date, and that deleting an
`aria-label` as redundant-looking silently unnames the field.
## What it does
`app-form-field` renders the label itself and then **moves that node into
the control's `label` slot** after the first render. Angular keeps owning the
node, so the translation and the required marker keep updating in place; Ionic
sees a slotted label and emits `aria-labelledby` against its own inner input:
```html
<input class="native-input" id="ion-input-0" name="name"
aria-labelledby="ion-input-0-label">
```
The alternative — setting the control's `label` *string* property — cannot
carry the marker as markup, and would have put an asterisk inside the
accessible name.
The control stays the caller's: projected, not rendered here. 574
`[(ngModel)]` bindings and the `name` that template-driven validity registers
by would otherwise all have changed shape, and `[disabled]="form.invalid"` on
92 submit buttons depends on that registration.
## The error is shown on request, not derived — and why
Deriving it from the projected control is the nicer API and I tried it.
`contentChild(NgModel)` resolves, and the control's `invalid` and `touched` are
both true at the point the error should appear, but a block conditioned on them
never renders. Measured, not assumed: removing the bindings makes the converted
form's two error cases fail, restoring them makes them pass.
The likely cause is that the only signal such a condition reads is the query
itself, which does not change when the control's plain getters do, so the
wrapper's view is never invalidated. **That mechanism is a guess and is written
down in the code as one.** The behaviour is not. So the caller passes the
condition it already has from its own template reference — one binding per
field instead of ten lines of markup, with the error's shape, placement and
`role="alert"` living in the wrapper. Deriving it is worth returning to; it was
not worth guessing at.
## The converted form
`office-form.component.ts`, as the issue suggests: **82 lines of template
out, 43 in**, for three of its four fields.
**Its spec passes untouched** — the issue names that as the strongest
available signal that the refactor is faithful, and I did not edit a single
assertion. That includes the one counting `required-marker` occurrences in the
rendered HTML, which the wrapper satisfies because the marker is still real
markup, just relocated.
The date triad is left alone. `ion-datetime` has no `label` property to slot
into, and it wants its own `app-date-field` — which #287 lists as a separate
component.
## The proof the issue asks for
`e2e/form-field-labelling.spec.ts` uses `getByLabel('Office Name')` to fill
the control and asserts the value lands. **The suite had zero `getByLabel`
calls before this**, which is itself the symptom: labels were not associated,
so nobody could write one. It also checks the select and the optional field the
same way.
## Also in scope, per the issue
`.app-form-container` lands in `src/styles/_common.scss`, with the width as
a custom property so a form overrides one value rather than re-declaring
padding and centring. 92 components still declare their own copy at five
different widths (600px ×54, 900px ×22, 800px ×9, 1000px ×4, 1200px ×3) —
converting them is two lines each and deliberately **not** done in bulk here.
`STYLE.md` taught the old shape, which is a large part of why it kept being
written. It now teaches this one and shows the old one as wrong, with the
reason, and notes that dates are not covered yet.
## Deliberately out
`app-select-field` turned out unnecessary — `ion-select` takes the same
`label` slot, so the one component serves both, which is a simplification worth
noting against the issue's three-component sketch. `app-date-field`, the other
92 containers, and the remaining ~150 forms are each their own change. No
decision is made here about template-driven vs signal forms; the point of
landing this first is that the question gets cheaper either way.
One thing found on the way: `src/app/ui` is where this had to live, not
`shared/components/` as the issue suggests. Every file in `shared/components/`
that imports Ionic is a *suppressed* lint violation under ADR 0005, so a new
one there would fail the build.
## Checks
- unit suite 327 files / 2809 tests; the converted form's 11 unchanged and
passing
- 6 new unit cases on the primitive, including that the label lands in the
control's slot and that the projected control keeps its `name`
- `lint` silent, `tsc` app + e2e clean, `i18n:check` clean, `format:check`
clean on tracked files
- `check:a11y-names`, `check:template-text`, `check:ui-primitives`,
`check:responsive`, `check:theme-contrast` all pass
- mocked e2e 1/1; an NG0100 the first run reported in this component was
mine (a field assigned from `afterNextRender`) and is fixed by making it a
signal — the run is clean now
--
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]
