fix(easycla): document headless hand-off open and rename sealed test - #2351
fix(easycla): document headless hand-off open and rename sealed test#2351ahmedomosanya wants to merge 1 commit into
Conversation
Reviewer notes on #2344: the call site should carry the same showHeader / ariaLabelledBy / contentStyle comments as lens-switcher, and the sealed-state spec should name the in-content close control. Signed-off-by: ahmedomosanya <aopeyemi@contractor.linuxfoundation.org>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR SummaryLow Risk Overview
The hand-off spec renames one test from dismissing via a header control to the in-content close control while preparing, matching the UI after the PrimeNG header was removed. Reviewed by Cursor Bugbot for commit e19f32d. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Documents the headless CCLA hand-off dialog configuration and aligns the sealed-state test name with the actual close control.
Changes:
- Explains dialog heading, accessibility labeling, and padding configuration.
- Renames the preparation-state test for accuracy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
org-easycla-sign-handoff.component.spec.ts |
Clarifies the tested close-control behavior. |
org-easycla-detail.component.ts |
Documents headless dialog configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@ahmedomosanya — thanks for the follow-up to #2344. This is a tight, comment-only patch that documents the headless hand-off dialog config and renames the sealed-state spec to match the in-content close control. The One issue blocks merge: the new Issue count
Bot reconciliation
Prior reviewCopilot posted a COMMENTED overview on 2026-09-11 with no inline findings. No prior formal approval or changes-requested review to track. Final decision🔴 Needs changes before approval — please correct the |
dealako
left a comment
There was a problem hiding this comment.
Review summary
🔴 Needs changes before approval
Solid follow-up documentation for the headless CCLA hand-off dialog. The showHeader and contentStyle comments and the spec rename look good.
Blocking (1): The ariaLabelledBy comment at org-easycla-detail.component.ts:403-405 describes behavior PrimeNG 20.4.0 does not implement — DynamicDialogConfig.ariaLabelledBy is declared but never bound in DynamicDialog's <p-dialog> template, so the config value is inert. Because this PR only adds comments, merging would entrench a false accessibility rationale at the call site.
See inline comment for proof and fix options.
Bots: Agree with Cursor/Copilot on low runtime risk; disagree that the ariaLabelledBy comment is accurate.
| const handoffRef = this.dialogService.open(OrgEasyclaSignHandoffComponent, { | ||
| // No PrimeNG header — the dialog body renders its own heading. | ||
| showHeader: false, | ||
| // Name the role="dialog" for assistive tech: with showHeader:false PrimeNG emits no |
There was a problem hiding this comment.
[blocking] Correct the ariaLabelledBy comment — config value is inert in PrimeNG 20.4.0
Issue: The comment claims that with showHeader:false PrimeNG "emits no generated title" and that setting ariaLabelledBy points the role="dialog" at the body's <h2>. Neither is true for the pinned dependency (primeng@20.4.0).
Proof: In PrimeNG 20.4.0, DynamicDialogConfig.ariaLabelledBy is documented on the config type but DynamicDialog's template forwards header, showHeader, contentStyle, etc. to <p-dialog> — there is no [ariaLabelledBy] binding, so ddconfig.ariaLabelledBy is never read. Dialog has no ariaLabelledBy @Input(); it computes its own ariaLabelledBy field via getAriaLabelledBy() (header !== null ? uuid + '_header' : null). With no header passed, undefined !== null is true, so a generated pn_id_N_header id can be bound via [attr.aria-labelledby] on the dialog root while the <span [id]="ariaLabelledBy"> lives inside the titlebar gated by *ngIf="showHeader" and is not rendered. The hand-off <h2 id="org-easycla-sign-handoff-heading"> is never referenced. Locally, org-easycla-detail.component.spec.ts:398 only asserts the mocked open() config echo — it cannot detect PrimeNG ignoring the property. The same pattern exists at lens-switcher.component.ts:143-145.
Why it matters: This is the CCLA signing hand-off modal (non-dismissible while preparing). Screen readers may announce an unnamed dialog because aria-labelledby references a non-existent element. This PR's sole deliverable is these comments — merging cements a false account and removes the prompt to fix accessibility.
Fix: Wire naming through a channel DynamicDialog actually forwards, e.g. pt: { root: { 'aria-labelledby': OrgEasyclaSignHandoffComponent.headingId } } on the open() config (confirm the rendered attribute resolves to the <h2>), then rewrite the comment to match. If keeping the diff comment-only, the comment must state that DynamicDialogConfig.ariaLabelledBy is inert in PrimeNG 20.4.0 rather than claiming it names the dialog.
Follow-up to #2344 (own ccla hand-off heading in the child): add the lens-switcher
showHeader/ariaLabelledBy/contentStylecomments on the CCLA hand-off open, and rename the sealed-state spec to the in-content close control.Refs #2344