opensource-joe commented on issue #355:
URL:
https://github.com/apache/fineract-backoffice-ui/issues/355#issuecomment-5311998215
@Aman-Mittal Writing the two decisions back as promised, now that #380 is up
and I have taken `products` and `system` far enough to know whether the
convention survives contact with them. It mostly does, with one correction I
would rather make myself than have found in review.
Grounded in 135 routes across the three files, not in the 21 that settled it.
## Decision 1: existing keys or new ones
**Take the key the page already uses for its own visible heading.** Not a
new key, and not the `nav.*` entry, except in the two cases below.
- The **section root** (`path: ''`) restates the `nav.*` key its section
entry uses in `app.routes.ts`, so the file reads on its own rather than relying
on inheritance. `clients` does this with `nav.clients`, as `groups` already did
with `nav.groups`.
- Everything else takes its page's heading key: `CLIENTS.ADD_IDENTIFIER`,
`SYSTEM.EDIT_DATA_TABLE`, `TAX_GROUPS.CREATE`, and so on.
**Correction to what I wrote in #380.** That description says the convention
needs no new translation keys, and offers it as an argument for the convention.
That was true of `clients` and it is not true in general, so please do not hold
me to it as stated.
The accurate version is a rule about **route shapes**, not about feature
areas:
| File | Routes | New keys needed |
|---|---|---|
| `clients` | 21 | 0 |
| `system` | 49 | 0 |
| `products` | 65 | 6 |
`system` is the larger file and needed nothing. `products` needed six. The
difference is not luck: **`system` has no `view/:id` route at all**, and
`products` has five. Every screen in `system` is a list, a configuration page
or a create/edit form, and each of those already renders its own name, so rule
2 always finds a key. A record view is the one shape where the visible heading
is the *record* rather than the *screen*, which is exactly when there is no key
to reuse.
So: **a file needs new keys if and only if it contains routes whose heading
is bound at runtime.** Anyone picking up a file can tell in advance which they
have.
## Decision 2: `view/:id`, `edit/:id`, and anything else the router
parameterises
`edit/:id` is not a special case at all. The form already renders "Edit X"
as its heading, so rule 2 applies unchanged: `CLIENTS.EDIT_CLIENT`,
`TAX_COMPONENTS.EDIT`, `SHARE_ACCOUNTS.EDIT`. Same for `create`.
`view/:id` is the real case, and it is worth being explicit because the
obvious reading of rule 2 gives the wrong answer.
Angular's static `title` cannot interpolate the record, so the route takes
**a generic key naming the screen type, and it must be specific to the
entity**. The trap is that these pages usually render `COMMON.DETAILS` or
`COMMON.OVERVIEW` in the card above the record's name, so following rule 2
literally is possible and produces:
```
Details · Fineract <- loan product
Details · Fineract <- savings account
Details · Fineract <- fixed deposit account
Details · Fineract <- recurring deposit account
```
which is this issue's own problem, one level down. `products` is where that
shows up, because it has five record views in one file.
What #380 and the two branches do instead:
| Route | Title | |
|---|---|---|
| `clients/view/:id` | `CLIENTS.DETAILS` | existed |
| `products/loan/view/:id` | `nav.loanProductDetails` | existed |
| `products/savings-accounts/view/:id` | `SAVINGS.ACCOUNT_DETAILS` | new |
| `products/fixed-deposits/view/:id` | `FIXED_DEPOSITS.ACCOUNT_DETAILS` |
new |
| `products/recurring-deposits/view/:id` |
`RECURRING_DEPOSITS.ACCOUNT_DETAILS` | new |
| `products/shares/view/:id` | `SHARE_ACCOUNTS.ACCOUNT_DETAILS` | new |
Same treatment for any route whose heading is bound at runtime even when it
is not a `view/:id`. Two examples already hit:
- `products/:accountType/:accountId/action/:command` picks its heading from
a command-to-key map, so it takes a new `ACTIONS.ACCOUNT_ACTION`.
- `system/codes/:codeId/values` binds `[title]="codeName()"`, the code's own
name, so it takes the existing generic `CODE_VALUES.TITLE`.
**The test that keeps this honest.** Both branches carry a spec asserting
that **no two routes in a file share a title**. That is the one that fails if
someone later takes the `COMMON.DETAILS` shortcut, and it is verified in the
failing direction, not just the passing one. I would suggest every file's PR
carries it, since a titling change that leaves two tabs identical has not done
its job.
## The `tellers` case you asked me to cover
`tellers/:tellerId/cashiers/:cashierId/transactions/:command`.
The good news is that **the depth is not the problem and needs no special
rule.** Three levels of parameter title exactly like one. Rule 2 reads the
page's heading and nothing about nesting enters into it.
The `:command` segment is the problem, and it comes with a collision that is
easy to walk into:
- `.../transactions` renders `TELLERS.CASHIER_TRANSACTIONS` ("Cashier
Transactions"). Rule 2, straightforward.
- `.../transactions/:command` picks `TELLERS.ALLOCATE_CASH` or
`TELLERS.SETTLE_CASH` depending on the parameter, so no static title can name
it.
The tempting move is to give the second one `TELLERS.CASHIER_TRANSACTIONS`
too, since it is the nearest generic. That collides with the list route and the
uniqueness test above will fail it. It needs **one new key**, singular,
something like `TELLERS.CASHIER_TRANSACTION` ("Cashier Transaction"). That is
the whole of the special handling that file needs, and it is the only new key
in it.
## Which of the remaining files this affects
Rough count of routes whose title cannot come straight from a heading, so
whoever picks a file knows what they are in for:
- **Need at least one new key:** `accounting`, `loans`, `centers`,
`organization`, `working-capital`, `calendars`, `meetings`, `fintech`,
`tellers`.
- **Should need none:** `admin`, `campaigns`, `interop`, `reporting`,
`security`, `settings`, `spm`, `tasks`, `transfers`.
Worth checking rather than trusting, since I derived it by pattern-matching
route paths rather than by reading all sixteen files.
## One thing I found and did not fix
Three list components in `products` carry hardcoded English headings rather
than translation keys: `fixed-deposits-list`, `recurring-deposits-list` and
`share-accounts-list` render `title="Fixed Deposit Accounts"` and similar.
`check-translations.mjs` cannot catch this, because it only validates strings
that are already keys. It is a real i18n bug but not a titling one, so those
three routes take the correct `nav.*` keys and I have left the headings alone.
Happy to file it separately if you want it tracked.
Also, and much smaller: `products/recurring` and `products/share` are the
*product* lists but render `nav.recurringDeposits` and `nav.shares`, which are
the *account* labels. Their titles use `nav.recurringDepositProducts` and
`nav.shareProducts`, so on those two the tab is right and the on-screen heading
is wrong.
---
**Status.** `clients` merged as #380. `products` is now up as **#382** and
`system` as **#383**, both applying the convention above. `accounting`, `loans`
and the 16 small files remain open for newcomers, with the three decisions here
settled so nobody has to re-derive them.
--
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]
