fix(Clickable): allow aria-haspopup to accept ARIA popup tokens, not just boolean - #3509
Merged
Merged
Conversation
…just boolean Clickable typed aria-haspopup as boolean, so consumers could not pass the literal "dialog" token — a Clickable opening a dialog was announced as having a menu popup (WCAG 4.1.2). Widen the type to React.AriaAttributes["aria-haspopup"], matching the existing pattern in Search/Button/IconButton. Type-only widening (strict superset of boolean), no runtime change; the value already passes through uncoerced at HEAD. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Code Review by Qodo
1. Type changes stay in the implementation file
|
Contributor
PR Summary by QodoSupport ARIA popup tokens in Clickable
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
orrgottlieb
approved these changes
Sep 8, 2026
Contributor
|
Code review by qodo was updated up to the latest commit 7dd759f |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+50
to
+54
| * Indicates the presence and type of popup associated with the element. | ||
| * Accepts the ARIA popup tokens (e.g. `"dialog"`, `"menu"`, `"listbox"`) in addition to a boolean, | ||
| * so assistive technology announces the correct popup type rather than defaulting to "menu". | ||
| */ | ||
| "aria-haspopup"?: boolean; | ||
| "aria-haspopup"?: React.AriaAttributes["aria-haspopup"]; |
Contributor
There was a problem hiding this comment.
1. Type changes stay in the implementation file 📘 Rule violation ⚙ Maintainability
ClickableProps is maintained in Clickable.tsx instead of a dedicated *.types.ts file. Changing the interface here keeps future prop changes split from the repository's expected type location and leaves the component without a dedicated type file.
Agent Prompt
## Issue description
`ClickableProps` is defined in the implementation file rather than a dedicated `*.types.ts` file, contrary to the component type organization requirement.
## Issue Context
Preserve the existing public API while moving the interface and updating imports or exports as needed.
## Fix Focus Areas
- packages/components/clickable/src/Clickable/Clickable.tsx[50-54]
- packages/components/clickable/src/Clickable/Clickable.types.ts[1-1]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 600817f |
Contributor
|
📦 Bundle Size Analysis ✅ No bundle size changes detected. Unchanged Components
📊 Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Clickabletypedaria-haspopupasboolean, so consumers could not type-legally pass the literal"dialog"token. AClickablethat opens a dialog was announced by assistive tech as having a menu popup instead of a dialog.WCAG: 4.1.2 Name, Role, Value (Level A).
Monday task: https://monday.monday.com/boards/18428773755/pulses/12987714364
Change
Widen the prop type in
ClickableProps:This matches the pattern already used by siblings in this monorepo —
Search(React.AriaAttributes["aria-haspopup"]),ButtonandIconButton(React.HTMLProps<HTMLButtonElement>["aria-haspopup"]). TheAriaAttributesform is used becauseClickablerenders an arbitraryelementType(defaultdiv), not a button. The"dialog"token is already used internally by Vibe (Info, all threeDropdowntriggers), so it's an established in-tree pattern.The runtime coercion (
!!D) that older builds had is already gone at HEAD — the value passes through uncoerced. This PR fixes the remaining defect: the type.Added 3 tests (
"dialog"renders verbatim; booleantruestill renders"true"; attribute omitted when not passed) and aWithDialogPopupStorybook story.Non-breaking
Type-only widening. The new union is a strict superset of
boolean, the default staysundefined, and no runtime line changed — every currently-legal call site stays legal. A runtime opt-in prop can't gate a TypeScript type, so none was added.Testing
lerna run build --scope=@vibe/clickable --include-dependencies→ 3 projects succeeded.yarn workspace @vibe/clickable test→ 2 files, 9 tests passed (6 pre-existing + 3 new). Existing snapshots unchanged.yarn workspace @vibe/clickable lint→ clean.Notes for reviewers (out of scope, worth follow-up items)
!!Dcoercion is still live in3.88.0/3.89.0-alpha(which the downstream consumer pins). This diff is against 4.x and fixes the type only; on 3.x the value would still coerce totrueat runtime. Either a 3.x backport of the runtime fix or a consumer move to 4.x is needed — a release-owner call.booleantyping exists atChips.tsx:115(defaultfalse), andMenuButton.tsx:377hardcodesaria-haspopup="true". Neither is this item's defect; flagging so the package fix isn't left half-applied.🤖 Generated with Claude Code