fix(frontend): name repository row actions - #286
Conversation
parthrohit22
left a comment
There was a problem hiding this comment.
Hi @adity982 , thanks for the clean, well-scoped fix and for the honest test notes (including the disclosed #179 timeout). The code change is correct: handlers are unchanged, and you handled the private-roadmap constraint properly by scoping alignment to #235 .
I am requesting changes as,
Blocking: the public WCAG baseline doc still lists #235 as an open violation.
The PR body says #235 was "removed from the accepted axe baseline findings" — that's true for accessibility.spec.ts, but docs/accessibility/WCAG_2_2_AA_BASELINE.md still records it as a live confirmed violation at line 106, and its own rule at line 110 states no confirmed violation may be left only in the report. As written, the public doc asserts a violation the code no longer has, and the "No visible documentation change is required" checklist item is therefore inaccurate. Please strike that row and add a resolved note, e.g.:
/repositories, success list | Every open/delete icon action lacks a repository-specific accessible name (button-name) … | #235
Resolved by #286 — open/delete actions now carry repository-specific accessible names (Open <repo>/Delete <repo>); icons marked decorative.
Then re-run npm run lint:frontend and npm --prefix apps/frontend test -- --run, and push to the same branch. The component test, E2E per-fixture assertions, and aria-hidden handling are all good as-is - this doc edit is the only blocker; approval follows once it lands.
keep up the good work.
Tested: read the full handler block in RepositoriesPage.tsx (onClick unchanged), the new component test, and the E2E delta; confirmed CI is running (CodeQL passed, CI in progress) and the fork-PR workflow-approval gate is the expected first-run behaviour.
Mapped against: the public WCAG 2.2 AA baseline doc (line 106 still lists #235; its own line 110 forbids a confirmed violation remaining only in the report) and the PR's stated acceptance criteria.
Signed-off-by: Aditya Datta <crazyme07071996@gmail.com>
46a22a2 to
f2feb59
Compare
|
Hi @adity982 , |
|
Implemented all requested follow-up on current head f2feb59: struck the #235 WCAG baseline row; added the explicit Resolved by #286 note; updated the PR checklist; revalidated the rebased head (frontend lint passed; full Vitest coverage passed serially, 42 files / 223 tests; hosted CI and CodeQL are green). GitHub reports no inline review threads on this PR, so there is no separate conversation control to resolve; the top-level requested-change blocker is fully addressed. Ready for re-review. |
parthrohit22
left a comment
There was a problem hiding this comment.
@adity982 — thanks for turning the doc fix around quickly. This is an approval.
What's resolved
- The blocking item is closed:
docs/accessibility/WCAG_2_2_AA_BASELINE.mdnow strikes the #235 row and records "Resolved by #286 — repository open/delete actions now carry repository-specific accessible names (Open <repository>/Delete <repository>), and their icons are marked decorative." The public doc no longer asserts a violation the code no longer has, and the "documentation updated" checklist item is now accurate. - The code change remains correct:
RepositoriesPage.tsxaddsaria-label={Open ${repo.name}}/Delete ${repo.name}and marks both Lucide iconsaria-hidden="true" focusable="false"; theonClickhandlers are unchanged. accessibility.spec.tsremoves the #235 axe exception and adds explicit per-fixturegetByRole('button', { name: 'Open <repo>' })assertions across allFIXTURES.repos— stronger than the prior exception.- The new component test asserts the accessible name and the decorative
aria-hiddenSVG on both actions.
Checks
- CI: success (run 31471319296). CodeQL: success (run 31471319305). The earlier
action_requiredruns were the expected fork first-run workflow-approval gate, now cleared. - Test disclosure is honest: the parallel Windows run hit worker-timeout flakiness, and you re-ran single-worker to a clean 42 files / 223 tests plus a green build. Good.
Minor (non-blocking) nit
The strike-through inside a markdown table cell may not render as strikethrough on GitHub for all viewers. If you want it unambiguous, consider deleting the row entirely rather than striking it — but the following "Resolved by #286" note already makes the state clear, so this is optional.
Nicely done. Approving.
Tested: read the full handler block in RepositoriesPage.tsx (onClick unchanged), the new RepositoriesPage.test.tsx, the E2E delta, and the WCAG_2_2_AA_BASELINE.md strike + resolved note; confirmed latest CI and CodeQL runs report success.
Mapped against: the public WCAG 2.2 AA baseline doc (line 106 now struck; line 110's "no confirmed violation left only in the report" rule satisfied) and the PR's stated acceptance criteria.
Summary
Give every repository-row open and delete action a repository-specific accessible name, hide the decorative icons from assistive technology, and pin the behavior in component and WCAG baseline coverage.
Linked issue
Closes #235
Roadmap alignment
What changed
Open <repository>andDelete <repository>labels to the existing row actions.Acceptance criteria completed
Testing performed
The branch was rebased onto current
upstream/devbefore these checks.Screenshots
Not applicable. This changes accessibility semantics without changing the visible UI.
Security and data considerations
None. No auth, persistence, request, or data-handling behavior changed.
Dependencies and blocked work
None.
Scope changes or remaining work
The public checkout does not include the private roadmap file, so the roadmap workstream wording above follows the alignment recorded in issue #235. No implementation work was deferred.
Contributor checklist
devupstream/devReviewer requested by the contribution guide: @parthrohit22