Aman-Mittal opened a new pull request, #568:
URL: https://github.com/apache/fineract-backoffice-ui/pull/568
## What and why
Continues ADR 0005 from #566: migrates 47 list screens onto
`app-button`/`app-icon`/`app-spinner`, clears the suppressions entries that
were actually fixable, and adds the regression cover the migration needed —
because the existing suite did not provide any.
Closes #567. Addresses the collapse asked for in #514.
## 1. Lint baseline: 634 → 577, across 365 → 358 files
**`unicorn/number-literal-case` was unsatisfiable.** Configured to demand
uppercase hex while Prettier rewrites hex to lowercase and `format:check` gates
CI — so those three entries could not be fixed by anyone. I only noticed
because my `0xff` → `0xFF` fix silently reverted when I ran Prettier. Aligned
the rule to lowercase; entries clear with no source change, and it still
catches `0Xff` and `1E3`.
Then seven mechanical ones: `isNaN` → `Number.isNaN`, a dead `import {} from
'@angular/router'`, a truthiness ternary, positional child traversal, three
object-literal defaults hoisted.
**The 14 `unicorn/no-array-sort` entries are deliberately untouched.** The
rule wants `Array#toSorted`; the project targets ES2022 and browserslist
resolves **Chrome 109**, one below where `toSorted` shipped. Raising the lib
would compile code that throws on a supported browser. `role-form.component.ts`
already documents this.
## 2. `app-button` can navigate
17 of the migrated call sites put `[routerLink]` on an `ion-button`; without
this none of them could move.
**My first implementation was wrong and the commit says so.** I bound
`[routerLink]` internally — the directive instantiates whether or not commands
are given and injects `ActivatedRoute`, so *every* button would have needed a
router in its injector. It broke seven unrelated `EntityDatatablesComponent`
tests, correctly. The router is now injected optionally and reached only when
`link` is set, with a second suite that renders with no router at all so the
property fails loudly if traded away.
What that gives up against `routerLink` is href middle-click — which these
call sites never had: `ion-button` renders an `<a>` only for `href`, so a
`routerLink` on one already produced a `<button>` with a click handler and no
href.
## 3. 47 screens migrated: vendor baseline 290 → 243
Each file now imports no Ionic at all, so those 47 entries are **gone rather
than reduced**.
Scope was chosen by what the primitives already express, not by file count.
Held back deliberately:
- **16 files** whose icons use `slot`, `class`, `color` or their own aria —
`app-icon` does not model these yet.
- **Every file where an attribute would move** from `ion-button` onto the
`app-button` host (`data-testid`, `title`, `id`), because e2e selectors say
`ion-button[data-testid=…]` and **I cannot run e2e here**.
- **`layout/` and `shared/`**, whose icons appear in the axe baseline in
`e2e/accessibility.spec.ts`; making them `aria-hidden` would leave those
entries stale with no way to confirm locally.
## 4. Regression cover — the part the suite was missing
Of the 47 migrated components, 36 have a sibling spec and **almost none of
those specs assert anything about a button**; only two test files in all of
`src/app/features` mention `ion-button` or `app-button`. A full-suite pass
proved the components still *construct*, not that their behaviour survived.
- **`scripts/check-ui-primitives.mjs`** — every `<app-button>` declares
`type`, and every one can be named. `type` is a required signal input, so a
missing one throws NG0950 only when that screen renders, which for most list
screens means in front of a user rather than in CI. Wired into `ci.yml`.
Verified against broken probes: missing `type` caught, no-text-no-label caught,
labelled icon-only allowed.
- **`ClientNotesListComponent`** had no spec at all; it now asserts
navigation commands, accessible name, click handler, icon, and the
`*appHasPermission` guard — the dropped guard being the failure with real
consequences. It reads button inputs rather than the DOM, because Ionic lifts
`aria-label` into its shadow root and the DOM behaviour belongs to the
primitive's own spec.
**Five mutations of the migrated template were each confirmed to fail it**:
removing the label, the click handler, the permission guard, the navigation,
and the icon.
## 5. `aria-allowed-attr` baseline (#514)
Four entries across two baselines read as four problems; they are one Ionic
defect whose axe fingerprint moves with the host's state classes. Collected
behind one constant with one comment. No entry added or removed. The upstream
report #514 also asks for is **not** done here — that is filing in another
project's tracker.
## Verification
- **1512 unit tests across 246 files pass.**
- `npm run lint` clean; `lint:prune` a no-op, so the baseline is exact
rather than padded.
- `test:eslint-rules`, `test:scripts`, `check:ui-primitives`,
`check:a11y-names`, `check:icons`, `check:responsive`, `i18n:check`,
`check:nav-ids`, `check:route-permissions`, `typecheck:e2e` all pass. Prettier
clean on every changed file.
- Negative checks throughout: the five template mutations above, and the
three probes against the new static check.
**Not run: e2e.** It needs browsers and a backend. This is the main residual
risk on a 47-file template refactor, and it is why files whose attributes would
move, and the axe-scanned shell, were held back rather than migrated. A
reviewer running the e2e suites is the check I could not perform.
## Screenshots
Not applicable. `app-button` renders the same `ion-button` with the same
vendor inputs the call sites passed before — `color`/`fill` are now derived
from `intent`/`emphasis` rather than written inline. No visual change intended.
## AI assistance (optional)
- Tool / model: Claude Opus 5, via Claude Code.
- Harness / workflow: assisted investigation, a scripted template migration,
and drafting. The Prettier conflict was reproduced directly; the browserslist
floor was read from `npx browserslist`; the migration was applied by script and
then verified by lint, the static checks, the full unit suite, and the mutation
probes above.
## Checklist
- [x] I did not hand-edit generated files under `src/app/api/`.
- [x] New component or service code uses the adapter boundary in
`src/app/core/adapters/` instead of direct browser globals or imperative
third-party APIs. — The migration moves 47 files off the vendor entirely.
- [x] User-facing strings use translation keys. — No strings added;
`[attr.aria-label]="… | translate"` became `[label]="… | translate"`.
- [x] I added or updated tests appropriate to this change, or explained why
tests were not needed.
- [x] UI workflow changes include suitable e2e coverage, including
real-backend testing where relevant. — Existing e2e is unchanged and still
applies; files whose selectors would have moved were deliberately excluded, as
described above. I could not run the suites locally.
- [x] Commits are signed — see [Commit
Signing](CONTRIBUTING.md#commit-signing) in CONTRIBUTING.md.
- [x] I followed the [AI-assisted contributions
guidance](CONTRIBUTING.md#ai-assisted-contributions).
--
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]
