FenjuFu commented on code in PR #540: URL: https://github.com/apache/fineract-backoffice-ui/pull/540#discussion_r4045966685
########## 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: Thanks for measuring both themes. I added a themed `--primary-text` token in `_common.scss`: `#2471a3` in light (5.30:1 on `#ffffff`) and `#3498db` in dark (5.29:1 on `#1e1e1e`), with the measurements in the token comment. The selected label uses it; the border indicator stays on `--primary-color`. ########## 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: Addressed all three in c233ae5: - `aria-controls` is emitted only on the selected tab, so no tab points at a panel that isn't rendered. When `value` matches no tab, no tab has it at all (new unit test). - Disabled tabs use `aria-disabled` instead of `disabled`, so they stay focusable and the strip always keeps exactly one tab stop, including when every tab is disabled. Arrow keys still skip them, and click/Enter is ignored. I updated the last unit test to cover this. - `EntityDatatablesComponent` now selects the first table with a `registeredTableName` and only renders the strip when there is at least one named table. A new test checks that the selected tab and panel point at each other. ########## 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: Recorded in both places in c233ae5. The `TAB_GROUP_SELECTOR` comment now says the selector is mid-migration: only the nested custom-fields strip is `app-tabs`, and migrating a record-view strip must move the selector in the same change. ADR 0005 rollout step 1 lists this as known debt, including the point that an unmatched tour step does not fail CI. Should the tours move to the `ui-tabs` test id in rollout step 1, or would you rather take that as a separate PR? -- 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]
