You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "opensource-joe (via GitHub)" <gi...@apache.org> on 2026/08/16 21:31:36 UTC
[PR] fix(a11y): give every clients route its own title [fineract-backoffice-ui]
opensource-joe opened a new pull request, #380:
URL: https://github.com/apache/fineract-backoffice-ui/pull/380
## What and why
The 21 routes in `clients.routes.ts` all inherited their tab from the section above them, so every page under `clients` read `Clients · Fineract`. Measured in a browser against this branch's own parent, all 21 of them:
```
/clients Clients · Fineract
/clients/create Clients · Fineract
/clients/view/1 Clients · Fineract
/clients/1/identifiers/edit/1 Clients · Fineract
/clients/1/collaterals/create Clients · Fineract
...21 of 21 identical
```
This is the first of the 21 files in #355. It is deliberately one feature area, so the naming convention can be settled here and reviewed once rather than 20 more times. `clients` was the right file to settle it on because it contains every shape the convention has to answer: an empty path, a record-scoped page, a page whose heading is a record's name, and two-level nested sub-resources.
Refs #355
## The convention
In the order the rules apply:
1. **The section root takes the same `nav.*` key its section entry uses in `app.routes.ts`**, restated rather than inherited, so the file reads on its own. `''` gets `nav.clients`, exactly as `groups.routes.ts` restates `nav.groups`.
2. **Every other route takes the key its page already uses for its own visible heading**, so the tab and the page agree.
3. **Where the heading is a record's name and cannot be a static key, the route takes a generic key for the screen type.** Only `view/:id` needs this. It uses `CLIENTS.DETAILS`, matching `GROUPS.GROUP_DETAILS`.
**No new translation keys.** All 21 already existed, because rule 2 selects a key the page had to have in order to render its own heading. That is a property of the convention rather than luck about this file, and it is the main argument for it: a correct title costs nothing to translate and cannot drift from the heading it was taken from.
| Route | Key | Tab |
|---|---|---|
| `''` | `nav.clients` | Clients |
| `search` | `CLIENT_SEARCH_V2.TITLE` | Advanced Client Search |
| `create` | `CLIENTS.CREATE_CLIENT` | Create Client |
| `edit/:id` | `CLIENTS.EDIT_CLIENT` | Edit Client |
| `view/:id` | `CLIENTS.DETAILS` | Client Details |
| `:clientId/identifiers/create` | `CLIENTS.ADD_IDENTIFIER` | Add Identifier |
| `:clientId/identifiers/edit/:id` | `CLIENTS.EDIT_IDENTIFIER` | Edit Identifier |
| `:clientId/addresses/create` | `CLIENTS.ADD_ADDRESS` | Add Address |
| `:clientId/addresses/edit/:id` | `CLIENTS.EDIT_ADDRESS` | Edit Address |
| `:clientId/family-members/create` | `CLIENTS.ADD_FAMILY_MEMBER` | Add Family Member |
| `:clientId/family-members/edit/:id` | `CLIENTS.EDIT_FAMILY_MEMBER` | Edit Family Member |
| `:clientId/notes/create` | `CLIENTS.ADD_NOTE` | Add Note |
| `:clientId/notes/edit/:id` | `CLIENTS.EDIT_NOTE` | Edit Note |
| `:clientId/documents/create` | `CLIENTS.ADD_DOCUMENT` | Add Document |
| `:clientId/documents/edit/:id` | `CLIENTS.EDIT_DOCUMENT` | Edit Document |
| `:clientId/charges` | `CLIENT_CHARGES.TITLE` | Client Charges |
| `:clientId/charges/create` | `CLIENT_CHARGES.CREATE` | Add Charge |
| `:clientId/collaterals` | `CLIENT_COLLATERAL.TITLE` | Client Collateral |
| `:clientId/collaterals/create` | `CLIENT_COLLATERAL.CREATE` | Add Collateral |
| `:clientId/collaterals/edit/:id` | `CLIENT_COLLATERAL.EDIT` | Edit Collateral |
| `:clientId/transactions` | `CLIENT_TRANSACTIONS.TITLE` | Client Transactions |
## Verification
**In a browser, against the real `en.json`.** A unit spec with a fake catalogue can only prove the wiring, not that the tabs read correctly, so both states were measured with Playwright against the `mocked` project. All 21 routes, after:
```
/clients Clients · Fineract
/clients/search Advanced Client Search · Fineract
/clients/create Create Client · Fineract
/clients/edit/1 Edit Client · Fineract
/clients/view/1 Client Details · Fineract
/clients/1/identifiers/create Add Identifier · Fineract
/clients/1/identifiers/edit/1 Edit Identifier · Fineract
/clients/1/addresses/create Add Address · Fineract
/clients/1/addresses/edit/1 Edit Address · Fineract
/clients/1/family-members/create Add Family Member · Fineract
/clients/1/family-members/edit/1 Edit Family Member · Fineract
/clients/1/notes/create Add Note · Fineract
/clients/1/notes/edit/1 Edit Note · Fineract
/clients/1/documents/create Add Document · Fineract
/clients/1/documents/edit/1 Edit Document · Fineract
/clients/1/charges Client Charges · Fineract
/clients/1/charges/create Add Charge · Fineract
/clients/1/collaterals Client Collateral · Fineract
/clients/1/collaterals/create Add Collateral · Fineract
/clients/1/collaterals/edit/1 Edit Collateral · Fineract
/clients/1/transactions Client Transactions · Fineract
```
The before block at the top of this description was produced by the same run with only `clients.routes.ts` reverted to this branch's parent, so the two are directly comparable. That harness was throwaway and is not in the diff; say the word if you would rather it shipped as a permanent e2e spec.
One thing worth recording, since it would have produced a confidently wrong result. Reading `document.title` straight after `page.goto` proves nothing here, and neither does waiting for it to differ from `index.html`'s: the strategy writes the app name first and the translations arrive after, so an early read catches the raw `app.shortTitle` key or a bare `Fineract`. The settled state for a titled route is `Page · Fineract`, so the check waits for the separator.
| | |
|---|---|
| `npx ng test fineract-backoffice-ui` | **1100 of 1100 SUCCESS**, real headless Chromium |
| `clients.routes.spec.ts` alone | 7 of 7 SUCCESS |
| `npm run lint` | exit 0, no new warnings |
| `npm run lint:prune` | exit 0 |
| `npm run format:check` | exit 0 |
| `node scripts/check-translations.mjs` | exit 0, 1532 keys checked |
**The guard was tested in three directions**, since a spec over data that only ever passes proves nothing:
| Change | Result |
|---|---|
| delete the title on `:clientId/identifiers/edit/:id` | 3 failed, naming the route |
| replace a key with the phrase `'Client Details'` | 1 failed, the key-shape test |
| point `collaterals/create` at `CLIENT_COLLATERAL.EDIT` | 1 failed, `CLIENT_COLLATERAL.EDIT · Fineract` where `Add Collateral · Fineract` was expected |
| restore | 7 of 7 SUCCESS |
Not exercised against a real Fineract. This adds a `title` to route definitions and changes no request, no behaviour and nothing rendered in the page body.
## Screenshots
Not applicable. The change is in the browser tab, and the measured titles above are the evidence a screenshot of the page body would not be.
## Two things noticed while in the file, neither changed here
**`:clientId/collaterals` has no guard.** It is the only route in the file with no `canActivate` and no `data.permissions`, where its two siblings `collaterals/create` and `collaterals/edit/:id` both carry `authGuard, permissionGuard`. That looks like an oversight rather than a decision, but it is a permissions question and not a titles one, so I have left it alone. Happy to open a separate issue, or a one-line PR if you already know what it should declare.
**The clients list heading and its nav entry disagree.** `clients-list.component.ts` renders `MODULES.CLIENTS_CONTRACTS` ("Clients & Contracts") while the nav item and now the tab say "Clients". I followed rule 1 rather than rule 2 there, on the grounds that a tab which does not match the nav item that opened it is worse for findability than one which does not match the heading. Worth flagging because it is the one place in the file where the two rules disagree.
## The question this PR is really asking
If the convention above is right, the remaining 20 files are mechanical and I will work through them in the grouping you prefer. If any of the three rules is wrong, this is the cheapest possible place to say so.
## 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. No new component or service code.
- [x] User-facing strings use translation keys. Every title is a key, and no new keys were needed.
- [x] I added or updated tests appropriate to this change.
- [x] UI workflow changes include suitable e2e coverage, including real-backend testing where relevant. No workflow changes. The browser measurement above was run against the `mocked` project; a real backend would exercise nothing this change touches.
- [x] Commits are signed.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(a11y): give every clients route its own title [fineract-backoffice-ui]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on PR #380:
URL: https://github.com/apache/fineract-backoffice-ui/pull/380#issuecomment-5311508559
> @Aman-Mittal Would you be able to approve the workflow runs on this one when you get a chance?
>
> Nothing has executed, so the checks list is empty rather than red, which reads as CI not having started rather than as something waiting on a person. Same gate as #365.
>
> What is waiting on it, all run on a clean checkout of this branch: `lint`, `lint:prune`, `format:check` and `check-translations` at exit 0, and the unit suite at 1100 of 1100 against real headless Chromium, with the new `clients.routes.spec.ts` at 7 of 7 on its own. The titles were also measured in a browser against the real `en.json`, before and after, which is the table in the description.
>
> No rush on the review itself. Two things worth flagging while you have this open, though, because one of them is a correction to what I wrote above.
>
> **The "no new translation keys" claim in the description is too strong.** It is true of `clients` and it is not true in general, so please do not take it as an argument for the convention as stated. Having since worked through `products` and `system`:
>
> | File | Routes | New keys |
> |---|---|---|
> | `clients` | 21 | 0 |
> | `system` | 49 | 0 |
> | `products` | 65 | 6 |
>
> The accurate version is a rule about route shapes rather than about feature areas. A file needs new keys exactly when it has routes whose heading is bound at runtime. `system` has no `view/:id` route at all, so rule 2 always finds a key across all 49. `products` has five record views, where the visible heading is the record rather than the screen, and those are precisely the routes with no key to reuse. I will write that up properly on #355 once this lands, along with the `tellers` case you asked about.
>
> **`products` and `system` are already built and held.** Both are committed on local branches, fully verified, and deliberately not pushed. The convention is what this PR is really asking you to agree, and turning up with three PRs before you had answered would have decided it for you. If any of the three rules is wrong, this is the cheapest possible moment to say so and I will redo the other two accordingly.
>
Thanks, looks ok for me.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(a11y): give every clients route its own title [fineract-backoffice-ui]
Posted by "opensource-joe (via GitHub)" <gi...@apache.org>.
opensource-joe commented on PR #380:
URL: https://github.com/apache/fineract-backoffice-ui/pull/380#issuecomment-5310371528
@Aman-Mittal Would you be able to approve the workflow runs on this one when you get a chance?
Nothing has executed, so the checks list is empty rather than red, which reads as CI not having started rather than as something waiting on a person. Same gate as #365.
What is waiting on it, all run on a clean checkout of this branch: `lint`, `lint:prune`, `format:check` and `check-translations` at exit 0, and the unit suite at 1100 of 1100 against real headless Chromium, with the new `clients.routes.spec.ts` at 7 of 7 on its own. The titles were also measured in a browser against the real `en.json`, before and after, which is the table in the description.
No rush on the review itself. Two things worth flagging while you have this open, though, because one of them is a correction to what I wrote above.
**The "no new translation keys" claim in the description is too strong.** It is true of `clients` and it is not true in general, so please do not take it as an argument for the convention as stated. Having since worked through `products` and `system`:
| File | Routes | New keys |
|---|---|---|
| `clients` | 21 | 0 |
| `system` | 49 | 0 |
| `products` | 65 | 6 |
The accurate version is a rule about route shapes rather than about feature areas. A file needs new keys exactly when it has routes whose heading is bound at runtime. `system` has no `view/:id` route at all, so rule 2 always finds a key across all 49. `products` has five record views, where the visible heading is the record rather than the screen, and those are precisely the routes with no key to reuse. I will write that up properly on #355 once this lands, along with the `tellers` case you asked about.
**`products` and `system` are already built and held.** Both are committed on local branches, fully verified, and deliberately not pushed. The convention is what this PR is really asking you to agree, and turning up with three PRs before you had answered would have decided it for you. If any of the three rules is wrong, this is the cheapest possible moment to say so and I will redo the other two accordingly.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(a11y): give every clients route its own title [fineract-backoffice-ui]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal merged PR #380:
URL: https://github.com/apache/fineract-backoffice-ui/pull/380
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org