Conversation
WalkthroughAdds a new CCUI Popover component (implementation, types, styles), a Vue plugin entrypoint, tests and docs, updates test scripts, adds a .gitignore rule, and removes an obsolete tooltip spec. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger
participant Popover
participant FloatingUI
participant Body
User->>Trigger: interact (click/hover/focus/contextmenu)
Trigger->>Popover: dispatch event
Popover->>Popover: emit before-show
Popover->>FloatingUI: request position (placement/arrow/offset)
FloatingUI-->>Popover: position + middlewareData
Popover->>Body: teleport popper (if enabled)
Popover->>Popover: render inside Transition
Popover->>Popover: emit show / after-enter
Note right of Popover: hide via escape, outside click, timeout, or trigger
User->>Popover: outside action / timeout
Popover->>Popover: emit before-hide
Popover->>Popover: Transition leave
Popover->>Body: unmount popper
Popover->>Popover: emit hide / after-leave
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/docs/components/popover/index.md (1)
547-575: Aligntrigger-keysdocs with actual keyboard valuesAfter fixing runtime, the documented default should reflect the real
KeyboardEvent.keyspace value (and ideally mention that'Space'/'Spacebar'are also accepted). Please update the table text accordingly; otherwise consumers will copy'Space'and wonder why it fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/ccui/package.json(1 hunks)packages/ccui/ui/popover/index.ts(1 hunks)packages/ccui/ui/popover/src/popover-types.ts(1 hunks)packages/ccui/ui/popover/src/popover.scss(1 hunks)packages/ccui/ui/popover/src/popover.tsx(1 hunks)packages/ccui/ui/popover/test/popover.test.ts(1 hunks)packages/docs/components/popover/index.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ccui/ui/popover/src/popover.tsx (2)
packages/ccui/ui/popover/src/popover-types.ts (1)
popoverProps(21-126)packages/ccui/ui/shared/hooks/use-namespace.ts (1)
useNamespace(30-44)
🔇 Additional comments (1)
packages/ccui/ui/popover/test/popover.test.ts (1)
318-320: Update ARIA assertion for unique popper idOnce each instance gets its own ID, this expectation must reflect the dynamic value (e.g., assert it starts with
ccui-popover__popper-). Otherwise the test will fail and we keep encouraging duplicate IDs.Use a pattern match such as
toMatch(/^ccui-popover__popper-/)(or read the actualaria-describedbyvalue from the rendered node and assert equality with the popper’sidattribute) so the test tracks the new runtime behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ccui/ui/popover/src/popover.tsx (2)
12-12: Thepersistentprop is defined but never used.The
persistentprop is declared in the component props but is not referenced anywhere in the implementation. Consider either implementing the intended functionality or removing the prop definition if it's not needed.
338-340: Document XSS risks forrawContentfeature.Using
innerHTMLwith therawContentprop bypasses Vue's built-in XSS protection. While this may be intentional for advanced use cases, ensure the documentation clearly warns users thatprops.contentmust be from a trusted source whenrawContentis enabled, as it can execute arbitrary HTML/scripts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.codeflicker/specs/tooltip-component-development.md(0 hunks).gitignore(1 hunks)packages/ccui/ui/popover/src/popover-types.ts(1 hunks)packages/ccui/ui/popover/src/popover.tsx(1 hunks)packages/ccui/ui/popover/test/popover.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .codeflicker/specs/tooltip-component-development.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ccui/ui/popover/test/popover.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ccui/ui/popover/src/popover.tsx (2)
packages/ccui/ui/popover/src/popover-types.ts (2)
popoverProps(21-126)PopoverProps(128-128)packages/ccui/ui/shared/hooks/use-namespace.ts (1)
useNamespace(30-44)
🔇 Additional comments (12)
.gitignore (1)
40-41: LGTM!The addition of
.claudeand.codeflickerto the ignore list is appropriate for tool-specific files.packages/ccui/ui/popover/src/popover-types.ts (2)
3-19: LGTM!The type definitions for
PopoverPlacement,PopoverEffect, andPopoverTriggerare comprehensive and well-structured. The inclusion ofcontextmenutrigger aligns with the PR objectives.
123-125: LGTM! Keyboard trigger keys fixed.The default
triggerKeysnow correctly uses' '(space character) instead of'Space', which matches the browser'sKeyboardEvent.keyvalue. This resolves the previous keyboard activation issue.packages/ccui/ui/popover/src/popover.tsx (9)
1-14: LGTM!Component structure, imports, and event declarations are well-organized. The comprehensive emit events support both lifecycle (
before-show,show,before-hide,hide) and animation hooks (before-enter,after-enter,before-leave,after-leave).
15-55: LGTM!The state management is well-structured:
- Proper separation of controlled/uncontrolled visibility modes
- Virtual trigger support correctly delegates to
virtualRefwhen enabled- Floating UI integration uses appropriate middleware (offset, flip, shift, arrow)
57-89: LGTM!The arrow positioning logic correctly calculates styles based on Floating UI's middleware data, and the timer cleanup helper properly handles all timer types.
91-136: LGTM!The show/hide logic is well-implemented:
- Properly handles both controlled and uncontrolled modes
- Correct event emission sequence
autoClosetimer is set after the popover is shown- Uses
nextTickto ensure positioning updates happen after DOM updates
179-207: LGTM! Keyboard handling properly normalized.The keyboard event handling now includes proper normalization:
- The
normalizeTriggerKeyhelper maps space key aliases (' ','space','spacebar') to a canonical form- Both the event key and prop trigger keys are normalized before comparison
- This resolves the previous review concern about inconsistent space key handling
138-178: LGTM!Event handlers are well-implemented:
- Each handler appropriately guards on trigger type
- Click and contextmenu handlers correctly toggle visibility
- Context menu handler properly prevents the default browser context menu
enterablesupport allows users to hover over the popover content without it closingAlso applies to: 208-217
221-316: LGTM!Lifecycle management is solid:
autoUpdateis properly set up and cleaned up- Virtual trigger event listeners are correctly managed in both mount/unmount
- Document-level listeners (outside clicks, Escape key) are dynamically added/removed based on visibility
- All cleanup is comprehensive in
onUnmounted
318-335: LGTM!The render helpers are well-structured:
renderArrowconditionally renders with proper stylingrenderHeaderprioritizes slots over props- Exposing the
hidemethod allows programmatic control from parent componentsAlso applies to: 348-350
352-426: LGTM!The render function is well-structured:
- Correctly handles both regular and virtual triggering modes
- Conditionally applies event handlers only when not using virtual triggers
- Properly integrates
Transitionfor animations with lifecycle hooks- Conditionally uses
Teleportbased on theteleportedprop- The
enterableprop correctly controls pointer events on the popper
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/ccui/ui/popover/src/popover.tsx (2)
297-303: Watcher only handlesvisiblebecoming true.The watch on
props.visibleonly updates positioning when the value becomes truthy. While theactualVisiblewatcher handles cleanup on hide, explicitly handling both transitions would make the logic clearer.watch(() => props.visible, (newVal) => { - if (newVal !== undefined && newVal) { + if (newVal !== undefined) { nextTick(() => { update() }) } })
29-35: ValidatevirtualRefwhenvirtualTriggeringis enabled.If
virtualTriggeringistruebutvirtualRefis not provided,actualTriggerRefreturnsundefined, causing positioning to fail silently. Add validation or a dev-mode warning.const actualTriggerRef = computed(() => { if (props.virtualTriggering && props.virtualRef) { return props.virtualRef } + if (props.virtualTriggering && !props.virtualRef) { + console.warn('[CPopover] virtualTriggering is enabled but virtualRef is not provided') + } return triggerRef.value })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ccui/ui/popover/src/popover.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ccui/ui/popover/src/popover.tsx (2)
packages/ccui/ui/popover/src/popover-types.ts (2)
popoverProps(21-126)PopoverProps(128-128)packages/ccui/ui/shared/hooks/use-namespace.ts (1)
useNamespace(30-44)
🔇 Additional comments (6)
packages/ccui/ui/popover/src/popover.tsx (6)
104-110: VerifyhideAfteris intentionally skipped for click triggers.Line 104 skips the
hideAfterdelay whentrigger === 'click', causing immediate hiding. Ensure this behavior is intentional and documented, as users might expect consistent delay behavior across all trigger types.
304-319: LGTM! Document event listeners are managed correctly.The watcher properly adds/removes document-level event listeners based on visibility, uses capture phase for correct outside-click detection, and ensures cleanup when the popover is hidden.
8-8: LGTM! Unique ID generation addresses previous review concerns.The counter-based approach generates a unique
popperIdfor each popover instance, fixing the duplicate ID issue flagged in earlier reviews. The ID is correctly used for both the popper element andaria-describedby.Also applies to: 16-16, 380-380, 404-404
182-210: LGTM! Key normalization addresses previous review concerns.The
normalizeTriggerKeyhelper correctly handles space key aliases (' ','space','spacebar') and normalizes both the event key andtriggerKeysprop before comparison, fixing the flaky space toggling issue from earlier reviews.
79-92: LGTM! Timer management is well-implemented.The
clearTimershelper prevents timer conflicts by clearing all pending timers before new operations. ThedoShowanddoHidefunctions correctly manage delays and theautoClosefeature, ensuring cleanup and proper sequencing.Also applies to: 112-139
355-429: LGTM! Render function correctly handles all popover modes.The render function properly distinguishes between virtual and non-virtual triggering modes, conditionally attaches events, emits animation lifecycle hooks, and supports both teleported and inline rendering. The structure is clear and maintainable.
| computed(() => actualTriggerRef.value), | ||
| popperRef, | ||
| { | ||
| placement: props.placement as any, |
There was a problem hiding this comment.
Remove as any type assertion to preserve type safety.
The as any assertion bypasses TypeScript checking. Import the correct Placement type from @floating-ui/vue and ensure PopoverPlacement aligns with it, or use a proper type assertion.
- placement: props.placement as any,
+ placement: props.placement,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| placement: props.placement as any, | |
| placement: props.placement, |
🤖 Prompt for AI Agents
In packages/ccui/ui/popover/src/popover.tsx around line 50, replace the unsafe
"as any" on placement with a proper type: import the Placement type from
"@floating-ui/vue", update or alias your PopoverPlacement to be compatible with
that Placement (or cast to Placement explicitly), then remove "as any" and
ensure the placement prop is typed/converted to Placement so TypeScript checking
is preserved and the file compiles.
1 participant
Popover 组件
📦 涉及文件
packages/ccui/ui/popover/src/popover-types.ts- 类型定义packages/ccui/ui/popover/src/popover.tsx- 组件实现packages/ccui/ui/popover/src/popover.scss- 样式文件packages/ccui/ui/popover/test/popover.test.ts- 测试用例packages/docs/components/popover/index.md- 组件文档✨ 新增功能
contextmenu右键菜单触发virtualTriggering和virtualRef属性，实现触发元素与展示内容分离autoClose属性，支持定时自动关闭transition动画，新增动画生命周期事件teleported属性，支持将弹出框传送到 bodytriggerKeys自定义键盘触发按键tabindex、persistent等配置项Summary by CodeRabbit
New Features
Documentation
Tests
Chores