Aman-Mittal commented on code in PR #540: URL: https://github.com/apache/fineract-backoffice-ui/pull/540#discussion_r3995634074
########## src/app/ui/tabs/tabs.component.ts: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { FocusKeyManager } from '@angular/cdk/a11y'; +import { + Component, + ElementRef, + inject, + input, + linkedSignal, + output, + viewChildren, +} from '@angular/core'; + +/** Values are application identifiers; labels are already translated by the caller. */ +export interface UiTab { + readonly value: string; + readonly label: string; + readonly disabled?: boolean; +} + +/** Manual-activation tabs: moving focus never fetches or replaces panel contents. */ +@Component({ + selector: 'app-tabs', + standalone: true, + template: ` + <div role="tablist" [attr.aria-label]="label()" data-testid="ui-tabs"> + @for (tab of tabs(); track tab.value; let index = $index) { + <button + #tabButton + type="button" + role="tab" + data-testid="ui-tab" + [id]="tabId(tab.value)" + [attr.aria-controls]="panelId()" + [attr.aria-selected]="tab.value === value()" + [attr.tabindex]="index === focusIndex() ? 0 : -1" + [disabled]="tab.disabled" + (focus)="focusIndex.set(index)" + (keydown)="onKeydown($event)" + (click)="select(tab)" + > + {{ tab.label }} + </button> + } + </div> + `, + styles: [ + ` + :host { + display: block; + min-width: 0; + } + [role='tablist'] { + display: flex; + overflow-x: auto; + border-bottom: 1px solid var(--border-color); + } + button { + flex: 0 0 auto; + min-height: 44px; + padding: var(--space-3) var(--space-4); + border: 0; + border-bottom: 2px solid transparent; + background: transparent; + color: var(--text-secondary); Review Comment: `--text-secondary` is not defined anywhere in the repository — this is its only occurrence. The declaration is therefore invalid at computed-value time, so `color` falls back to `inherit` and unselected tabs render at full `--text-color` rather than the muted tone intended here. The de-emphasis never appears in either theme. The repo's token for this is `--text-muted` (`src/styles/_common.scss:33`, redefined for dark at `:92`). ```suggestion color: var(--text-muted); ``` ########## eslint.config.js: ########## @@ -252,6 +242,20 @@ module.exports = tseslint.config( 'no-restricted-properties': 'off', }, }, + { + // UI implementations may name their vendor, but retain the Material and i18n boundaries. + files: ['src/app/ui/**/*.ts', 'src/app/testing/ionic-testing.ts'], + rules: { + 'no-restricted-imports': [ + 'error', + { + patterns: restrictedImportPatterns.filter( + (pattern) => !pattern.group.includes('@ionic/angular'), + ), + }, Review Comment: This filter drops the entire `@ionic/angular` pattern for `src/app/ui/**` and `src/app/testing/ionic-testing.ts`, which also removes ADR 0003's ban on Ionic's **imperative controllers** in those paths. Before this PR the controller restriction applied to every file with no exemption. Confirmed by running ESLint against the new config: ``` src/app/ui/probe.ts <- ModalController from @ionic/angular/standalone: ALLOWED src/app/ui/probe.ts <- AlertController from @ionic/angular/standalone: ALLOWED src/app/ui/probe.ts <- LoadingController from @ionic/angular: ALLOWED src/app/features/probe.ts <- ModalController from @ionic/angular/standalone: BLOCKED ``` ADR 0005 states "Imperative controllers still use OVERLAY" and "Existing OVERLAY, I18N, STORAGE and DOWNLOAD adapters remain in force", so the config is now weaker than the decision it implements. A UI primitive that needs a modal should still go through the OVERLAY adapter rather than reaching for `ModalController`. Suggest narrowing the exemption to components only, rather than removing the Ionic entry — keep the controller-scoped `importNames` restriction that the old config had: ```js patterns: [ ...restrictedImportPatterns.filter((pattern) => !pattern.group.includes('@ionic/angular')), { group: ['@ionic/angular', '@ionic/angular/*'], importNames: [ 'ModalController', 'ToastController', 'AlertController', 'LoadingController', 'ActionSheetController', 'PopoverController', ], message: "Use the OVERLAY adapter from 'app/core/adapters' instead of Ionic's controllers, inside src/app/ui as well. See DOCS/adr/0003-adapter-boundary.md.", }, ], ``` ########## scripts/ui-boundary.test.mjs: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { ESLint } from 'eslint'; + +const eslint = new ESLint(); +async function importErrors(filePath, module, symbol) { + const [result] = await eslint.lintText( + `import { ${symbol} } from '${module}'; export const imported = ${symbol};`, + { filePath }, + ); + return result.messages.filter((message) => message.ruleId === 'no-restricted-imports'); +} + +test('new feature Ionic imports are rejected without suppression', async () => { + assert.equal( + (await importErrors('src/app/features/probe.ts', '@ionic/angular/standalone', 'IonButton')) + .length, + 1, + ); +}); +test('UI implementations may use Ionic but cannot bypass other adapter boundaries', async () => { + assert.equal( + (await importErrors('src/app/ui/probe.ts', '@ionic/angular/standalone', 'IonButton')).length, + 0, + ); + assert.equal( + (await importErrors('src/app/ui/probe.ts', '@angular/material/button', 'MatButton')).length, + 1, + ); + assert.equal( + (await importErrors('src/app/ui/probe.ts', '@ngx-translate/core', 'TranslateService')).length, + 1, + ); +}); Review Comment: The test name promises UI implementations "cannot bypass other adapter boundaries", but it only asserts Material and ngx-translate — which is why the controller gap in `eslint.config.js` goes unnoticed. Worth closing the loop here so the assertion matches the name: ```js assert.equal( (await importErrors('src/app/ui/probe.ts', '@ionic/angular/standalone', 'ModalController')) .length, 1, ); ``` This currently returns `0`. With the `importNames` entry restored in the `src/app/ui/**` override it returns `1`, and the OVERLAY boundary stays enforced across the whole tree as the migration adds primitives that need dialogs. ########## DOCS/adr/0005-ui-boundary.md: ########## @@ -0,0 +1,119 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +# ADR 0005: An application-owned UI and test boundary + +- **Status:** Proposed; first tab migration implemented for review +- **Date:** 2026-09-10 +- **Discussion:** [#530](https://github.com/apache/fineract-backoffice-ui/issues/530) +- **Scope:** One UI implementation at a time; incremental migration, not a second component library + +## Problem and decision + +ADR 0003 isolates imperative dependencies but deliberately leaves template components outside +the boundary. A library swap still changes hundreds of templates, form-value semantics, and +browser locators. This proposal extends that boundary to `src/app/ui/` and to the test contract. +It does not declare the overall migration complete. + +Features import app-owned components from `src/app/ui/`; their public inputs, outputs, projected +content, ARIA and value contracts name application concepts. Ionic may be used inside this +directory while a primitive is being implemented. Behavioural primitives use CDK and native +elements where possible. Existing OVERLAY, I18N, STORAGE and DOWNLOAD adapters remain in force. +No dependency is added and no second vendor runs alongside Ionic. + +## Public contracts for the whole boundary + +| Tier | App-owned contract | Acceptance before migrating consumers | +| ------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Behavioural: tabs, popups, dialogs | Values and dismissal reasons, focus entry/return, keyboard behaviour, roles and accessible names; no vendor events or controller handles | Keyboard-only and pointer operation; disabled/empty/dynamic states; nested overlays; focus restored after close; browser checks at desktop and narrow widths | +| Form: input, select, date, checkbox/toggle | Angular ControlValueAccessor; `writeValue` never emits; disabled/touched/validation state; select preserves primitive identifiers; dates are ISO calendar strings, not UTC instants; explicit null/empty semantics | Shared CVA contract tests with reactive forms and ngModel; programmatic writes, reset, blur, disabled state; number/string identity and negative-timezone date round trips; the same browser helper against both implementations | +| Cosmetic: cards, buttons, icons, layout | Intent, label, disabled/busy state, content slots and app design tokens; buttons declare submit versus ordinary action | Accessible names, form submission, projected content, theme/density and responsive visual checks | + +The form migration uses explicit value types: text is `string` (empty is `''`), numeric +input is `number | null`, a single select is `string | number | null` without coercion, +a multi-select is a readonly array of those non-null identifiers, and checkbox/toggle values +are booleans. Calendar dates are valid `YYYY-MM-DD | null`; adapters must not round-trip them +through UTC. Defaults, reset, required validation and backend conversion remain feature +responsibilities. Every CVA shares disabled, blur/touched, `aria-invalid`, error-description +and label tests; replacing the renderer must not change submitted payloads. + +New primitives use `app-*` selectors. Existing `ion-*` selectors remain only in unmigrated +templates; no compatibility directive masquerades as an Ionic component. A consumer migration +updates its import, template and test helper together. The ~770 form bindings are migrated by +control type with contract tests, rather than being silently reinterpreted by a selector alias. + +Tests consume roles, accessible names, selected/expanded/disabled state and stable `data-testid` +scopes. They never inspect vendor shadow DOM, CSS classes or `CustomEvent.detail`. Helpers in +`e2e/utils/ui-locators.ts` are the new seam. Existing Ionic helpers stay for unmigrated controls; +do not extend them for app-owned primitives. Stable hooks also apply to guided tours. Review Comment: On "Stable hooks also apply to guided tours" — worth recording the concrete debt here, since it is invisible from this diff. `src/app/core/services/guidance.service.ts:63` sets `TAB_GROUP_SELECTOR = 'ion-segment'`, used by five tours. Nothing breaks in this PR: the record-view outer strips are still `ion-segment` and match ahead of the nested custom-fields strip in DOM order. But the doc comment above it now reads falsely — > All sixteen components with a tab strip render an `ion-segment`, so that is what the step points at. That comment exists precisely because the previous selector (`.tab-group`) silently matched nothing after the Material port and four tours pointed at empty space for some time. The same failure mode returns the moment rollout step 1 migrates a record-view strip — silently, since a tour step that matches nothing does not fail CI. Either update the comment now to say the selector is mid-migration, or promote the tours to a `data-testid` hook as part of step 1 rather than after it. ########## src/app/ui/tabs/tabs.component.ts: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { FocusKeyManager } from '@angular/cdk/a11y'; +import { + Component, + ElementRef, + inject, + input, + linkedSignal, + output, + viewChildren, +} from '@angular/core'; + +/** Values are application identifiers; labels are already translated by the caller. */ +export interface UiTab { + readonly value: string; + readonly label: string; + readonly disabled?: boolean; +} + +/** Manual-activation tabs: moving focus never fetches or replaces panel contents. */ +@Component({ + selector: 'app-tabs', + standalone: true, + template: ` + <div role="tablist" [attr.aria-label]="label()" data-testid="ui-tabs"> + @for (tab of tabs(); track tab.value; let index = $index) { + <button + #tabButton + type="button" + role="tab" + data-testid="ui-tab" + [id]="tabId(tab.value)" + [attr.aria-controls]="panelId()" Review Comment: Minor, on edge states in the ARIA contract: `aria-controls` is emitted unconditionally, but the consumer renders the `role="tabpanel"` element only under `@if (activeTable(); as dt)`. Before a selection resolves — and in `EntityDatatablesComponent` the tabs render as soon as `datatables()` is non-empty — every tab points at an ID that is not in the document. Two related cases, both reachable through the same path: when `value()` matches no tab, no button carries `aria-selected="true"`; and when every tab is disabled, `focusIndex` is `-1` so the strip has no tab stop at all (asserted as intended at the end of the unit test). APG expects a tablist to keep exactly one tab stop and one selected tab, so a screen-reader user can land on the strip and find out where they are. `EntityDatatablesComponent` also sets `activeTable` to `data[0]` unconditionally while `tableTabs` filters on `registeredTableName` — so a first table without a name yields a rendered panel whose `aria-labelledby` resolves to nothing, with no tab selected. Narrow, but it is the same class of gap and the non-null assertions in the template already acknowledge the field is optional. ########## src/app/ui/tabs/tabs.component.ts: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { FocusKeyManager } from '@angular/cdk/a11y'; +import { + Component, + ElementRef, + inject, + input, + linkedSignal, + output, + viewChildren, +} from '@angular/core'; + +/** Values are application identifiers; labels are already translated by the caller. */ +export interface UiTab { + readonly value: string; + readonly label: string; + readonly disabled?: boolean; +} + +/** Manual-activation tabs: moving focus never fetches or replaces panel contents. */ +@Component({ + selector: 'app-tabs', + standalone: true, + template: ` + <div role="tablist" [attr.aria-label]="label()" data-testid="ui-tabs"> + @for (tab of tabs(); track tab.value; let index = $index) { + <button + #tabButton + type="button" + role="tab" + data-testid="ui-tab" + [id]="tabId(tab.value)" + [attr.aria-controls]="panelId()" + [attr.aria-selected]="tab.value === value()" + [attr.tabindex]="index === focusIndex() ? 0 : -1" + [disabled]="tab.disabled" + (focus)="focusIndex.set(index)" + (keydown)="onKeydown($event)" + (click)="select(tab)" + > + {{ tab.label }} + </button> + } + </div> + `, + styles: [ + ` + :host { + display: block; + min-width: 0; + } + [role='tablist'] { + display: flex; + overflow-x: auto; + border-bottom: 1px solid var(--border-color); + } + button { + flex: 0 0 auto; + min-height: 44px; + padding: var(--space-3) var(--space-4); + border: 0; + border-bottom: 2px solid transparent; + background: transparent; + color: var(--text-secondary); + font: inherit; + cursor: pointer; + } + button[aria-selected='true'] { + color: var(--primary-color); + border-bottom-color: var(--primary-color); + } Review Comment: The selected tab's **label** is body-sized text in `--primary-color`. On `--card-bg` that is `#3498db` on `#ffffff` ≈ 3.2:1, short of the 4.5:1 WCAG AA minimum for body text. Dark theme is fine (`#3498db` on `#1e1e1e` ≈ 5.2:1). The `border-bottom-color` on the next line is a non-text indicator at 3:1 and is fine as-is. The token comment in `_common.scss` does say "Keep `--primary-color` for accents, borders and text on light backgrounds", but that clause sits directly beneath its own measurement of the same ratio at 3.15:1 — it holds for accents and borders, not for text. Worth flagging that `--primary-strong` is **not** a drop-in fix: it is not redefined in the `[data-theme='dark']` block, and `#2471a3` on `#1e1e1e` is ≈3.2:1, so a blanket swap trades the light-theme failure for a dark-theme one. This needs a themed token, the way `--guidance-highlight-color` is already themed in `_common.scss` for exactly this reason ("one colour cannot serve both"). Given ADR 0005 makes "dark mode, focus visibility, touch target size and overflow ... part of each primitive's acceptance", the first primitive seems like the right place to establish that token. -- 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]
