FenjuFu commented on code in PR #540: URL: https://github.com/apache/fineract-backoffice-ui/pull/540#discussion_r4045965541
########## 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: Good catch, thanks. Fixed in c233ae5: unselected tabs now use `--text-muted` (5.74:1 light, 7.69:1 dark). Disabled tabs no longer share that colour; they are dimmed with opacity instead, so the two states stay distinguishable. ########## 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: Agreed, the override was broader than ADR 0005 allows. c233ae5 keeps the component exemption but adds back the controller-scoped `importNames` entry for `src/app/ui/**` and `ionic-testing.ts`, as you suggested. `src/app/ui` and the testing helper still lint clean with it. ########## 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: Added in c233ae5. The test now asserts `ModalController` from both `@ionic/angular` and `@ionic/angular/standalone` is rejected in `src/app/ui/probe.ts`. It returned 0 before the ESLint change and returns 1 after. -- 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]
