opensource-joe commented on PR #365: URL: https://github.com/apache/fineract-backoffice-ui/pull/365#issuecomment-5317750798
@Aman-Mittal Rebased onto `eb90a7d`. The PR now reads `MERGEABLE`. Four commits, all still signed. Two things in the rebase were judgement calls rather than mechanical, so I would rather flag them than have you find them in review. ## 1. The permission directive rename Most conflicts were the same shape: `main` had moved icon-only buttons from `*appHasPermission="'X'"` to `appRequiresPermission="X"`, and in a few places renamed the permission itself, for example `UPDATE_CLIENTFAMILYMEMBER` to `UPDATE_FAMILYMEMBERS` and `UPDATE_DELINQUENCYRANGE` to `UPDATE_DELINQUENCY_RANGE`. I resolved every one of those by taking your version verbatim and re-applying only the added `[attr.aria-label]` line. Nothing from `main` was reverted. Same count as before the rebase, 21 labels across the six conflicted files, and `loan-documents-tab.component.ts` followed its move to `shared/components/entity-documents/entity-documents.component.ts`, where the labels now use the `appTranslate` pipe that file switched to rather than the old `translate`. `npm run check:a11y-names` passes, which is the real check on that resolution: if the rebase had dropped a label, the guard added in this PR would fail. ## 2. The tooltip spec, which you have since written yourselves This is the one worth a look. `main` now contains `tooltip.directive.spec.ts` with four specs, so this landed as an add/add conflict. **I kept your file and added to it rather than replacing it.** Your four specs are untouched, character for character, including the cross-host case that mine did not cover. The only edits to your part are the import line and the test component body, both needed to add a third host with signal-driven text. On top of that I added eight specs from this branch that yours does not cover: the pre-delay window, `role="tooltip"` plus the `aria-describedby` wiring, focus as well as hover, dismissal on Escape, cancelling a pending tooltip, clearing the description on leave, and empty text. The one that matters for this PR is `describes the host and never names it`. It asserts that the directive sets `aria-describedby` and does **not** set `aria-label`, which is the whole premise of the change: a description is not an accessible name, so an icon-only button still needs its own label. That is the boundary `check-a11y-names.mjs` enforces, and pinning it at the directive keeps the two honest about each other. I converted my specs from `fakeAsync`/`tick` to your `jasmine.clock()` harness so the file has one style throughout. Happy to drop any of the eight if you would rather keep that file lean. ## Verified On a clean checkout of the rebased branch: - `npm run check:a11y-names`, "Every icon-only `<ion-button>` has an accessible name" - `npm run lint`, clean - `npm run format:check`, clean - `npm test`, **1135 of 1135 passing** The workflow approval gate re-armed with the push, so the checks list will read empty until someone approves the runs. -- 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]
