feat: enhance event handling in suggestion components#117
feat: enhance event handling in suggestion components#117gene9831 merged 4 commits intoopentiny:release/v0.2.xfrom
Conversation
- Added `onItemClick`, `onClickOutside`, `onOpen`, and `onClose` event props to `SuggestionPopover` and `DropdownMenu` for improved flexibility. - Updated `SuggestionPill` actions to utilize new event props, replacing deprecated event handling. - Refactored `PillButtonWrapper` to support new event props for better integration with suggestion actions. - Improved handling of click events and visibility toggling in `SuggestionPills` and related components.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update refactors event handling across suggestion pills, dropdown menus, and popover components. It introduces explicit event handler props in type definitions, deprecates legacy event objects, and centralizes outside click logic using ignore options. Demo files and internal logic are updated to utilize the new event handler approach, improving consistency and control. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PillsComponent
participant Popover
participant DropdownMenu
User->>PillsComponent: Click pill with menu/popover
PillsComponent->>Popover: (if type is popover) Show, bind onOpen/onClose/onClickOutside
PillsComponent->>DropdownMenu: (if type is menu) Show, bind onItemClick/onClickOutside
Popover-->>PillsComponent: onOpen/onClose/onClickOutside callbacks
DropdownMenu-->>PillsComponent: onItemClick/onClickOutside callbacks
PillsComponent->>PillsComponent: Update menu visibility, log events
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
packages/components/vite.config.ts (1)
53-56: Keep alias key consistent with TSpathspattern
tsconfig.jsonuses the pattern~/*, whereas Vite is configured with the bare key"~".
The current setup works at runtime, but TypeScript assumes the slash after the tilde (~/foo). To avoid subtle mismatches (e.g. teammates writingimport '~foo'which TS will not resolve), align them:- alias: { - '~': fileURLToPath(new URL('./src', import.meta.url)), - }, + alias: { + '~': fileURLToPath(new URL('./src', import.meta.url)), // for imports like '~/xyz' + '~/': fileURLToPath(new URL('./src', import.meta.url)), // optional: exact match for '~/' + },Alternatively, change the TS pattern to
"~": ["src"]if you prefer omitting the slash altogether.packages/components/src/suggestion-popover/index.type.ts (1)
65-86: Deprecation JSDoc is good – surface it to toolingYou added
@deprecatedtags, but TypeScript will only warn if the symbol itself is marked/** @deprecated */ export interface ....
Consider duplicating the tag on the interface declaration (not just inside each property) so IDEs flag usage immediately.packages/components/src/flow-layout-buttons/index.vue (1)
116-122: Redundant ignore target & potential hover edge-case
onClickOutside()already treats the first argument (dropDownRef) as an ignored element.
Passing it again insideignoreis unnecessary.Additionally, when
showMoreTrigger === 'hover', users might move the mouse frommoreButtoninto the dropdown without clicking.
Becauseignoreonly containsmoreButtonRef, a mousedown inside the button followed by a mouseup inside the dropdown will close it immediately.
Consider addingmoreButtonRefanddropDownReftoignoreonly when trigger = 'hover'.-{ ignore: [dropDownRef, moreButtonRef] }, +{ ignore: [moreButtonRef] },or compute the array conditionally.
packages/components/src/action-group/ActionGroup.vue (1)
67-73: Good simplification – one small nitNice move to
onClickOutsidewithignore.
Like the previous file,dropDownRefis already excluded implicitly, soignore: [moreBtnRef]is sufficient.Not critical, but trimming it avoids confusion.
packages/components/src/dropdown-menu/index.type.ts (2)
30-33: Interface name leaks implementation details
DropdownMenuEventPropsduplicates emit names but adds the “on” prefix.
Consider renaming to simplyDropdownMenuEventsand dropping the deprecated map sooner; keeping two near-identical interfaces increases surface area without gain.
35-44: Deprecation comments need a removal plan
DropdownMenuEventsis marked@deprecatedbut still exported. Add a TODO with a targeted removal version to avoid indefinite coexistence.packages/components/src/suggestion-pills/index.type.ts (1)
20-29: Consider makingslotssymmetrical
PopoverActionoffers aslotsfield whereasMenuActiondoes not. If custom slots might be useful for menu actions later, adding the field now keeps the interfaces parallel and avoids a breaking change later.packages/components/src/suggestion-popover/index.vue (2)
150-168:emitClickTriggerEventsemits reversed semanticsWhen you close via outside-click you first set
show.value = falseand then callemitClickTriggerEvents().
Inside the helper you emit'open'whenshow.valueis truthy,'close'otherwise.
This is correct, but less obvious than emitting explicitly. Consider renaming toemitVisibilityEventand adding a short comment to avoid mis-reading the logic.
176-190: Duplicate close emission paths
handleCloseemits'close'directly, whereashandleToggleShow/handleItemClickroute throughemitClickTriggerEvents().
For consistency (and for potential centralisation of side-effects) call the helper here as well.- show.value = false - emit('close') + show.value = false + emitClickTriggerEvents()docs/demos/suggestion/pills-popper-config.vue (1)
128-129: Guard againstshowAllRefbeing undefined
event.composedPath().includes(showAllRef.value?.$el)is safe, but it might be clearer (and faster) to exit early when the ref is unset.-if (event.composedPath().includes(showAllRef.value?.$el)) { +const switchEl = showAllRef.value?.$el +if (switchEl && event.composedPath().includes(switchEl)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/demos/suggestion/pills-popper-config.vue(3 hunks)docs/demos/suggestion/popover-trigger.vue(2 hunks)packages/components/src/action-group/ActionGroup.vue(1 hunks)packages/components/src/dropdown-menu/index.type.ts(1 hunks)packages/components/src/dropdown-menu/index.vue(1 hunks)packages/components/src/flow-layout-buttons/index.vue(1 hunks)packages/components/src/suggestion-pills/components/PillButtonWrapper.vue(2 hunks)packages/components/src/suggestion-pills/index.type.ts(1 hunks)packages/components/src/suggestion-pills/index.vue(2 hunks)packages/components/src/suggestion-popover/index.type.ts(1 hunks)packages/components/src/suggestion-popover/index.vue(3 hunks)packages/components/tsconfig.json(1 hunks)packages/components/vite.config.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
packages/components/src/dropdown-menu/index.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:0-0
Timestamp: 2025-05-27T03:45:56.392Z
Learning: In Vue components using VueUse's onClickOutside, calling stopPropagation() in the onClickOutside handler can prevent subsequent click event handlers on the same element from being triggered. This means that clicking a trigger element while a popover is open can successfully close the popover without immediately reopening it, even when the onClickOutside only listens to the popover element and not the trigger element.
docs/demos/suggestion/popover-trigger.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:131-133
Timestamp: 2025-05-27T03:35:11.008Z
Learning: In the SuggestionPopover component (packages/components/src/suggestion-popover/index.vue), the click handler can be bound unconditionally because the `show` computed property has a custom setter that prevents state mutations when `props.trigger === 'manual'`. This design centralizes trigger mode logic in the computed property rather than requiring conditional checks in event handlers.
packages/components/src/suggestion-popover/index.vue (2)
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:131-133
Timestamp: 2025-05-27T03:35:11.008Z
Learning: In the SuggestionPopover component (packages/components/src/suggestion-popover/index.vue), the click handler can be bound unconditionally because the `show` computed property has a custom setter that prevents state mutations when `props.trigger === 'manual'`. This design centralizes trigger mode logic in the computed property rather than requiring conditional checks in event handlers.
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:0-0
Timestamp: 2025-05-27T03:45:56.392Z
Learning: In Vue components using VueUse's onClickOutside, calling stopPropagation() in the onClickOutside handler can prevent subsequent click event handlers on the same element from being triggered. This means that clicking a trigger element while a popover is open can successfully close the popover without immediately reopening it, even when the onClickOutside only listens to the popover element and not the trigger element.
🔇 Additional comments (5)
docs/demos/suggestion/popover-trigger.vue (1)
3-10: Demo looks good – showcases the new events clearlyThe extra console logs are helpful for users exploring the API.
packages/components/src/dropdown-menu/index.vue (1)
51-58: ```shell
#!/bin/bashShow props definitions and comments in dropdown-menu index.type.ts
sed -n '1,200p' packages/components/src/dropdown-menu/index.type.ts
</details> <details> <summary>packages/components/src/suggestion-pills/index.type.ts (1)</summary> `10-18`: **Good split between popover and menu actions** Separating `SuggestionPillPopoverAction` and `SuggestionPillMenuAction` makes the union easier to narrow and improves IntelliSense. LGTM. </details> <details> <summary>packages/components/src/suggestion-pills/index.vue (2)</summary> `16-17`: **Good use of defineModel for v-model support!** The implementation correctly uses Vue 3.3+ defineModel macro for cleaner two-way binding. --- `33-38`: **Fix inverted logic in floatingItems computed property.** The floating items should be displayed when overflow exists and `showAll` is true. Apply this diff to fix the logic: ```diff const floatingItems = computed(() => { - if (!hasShowMoreBtn.value || !showAll.value) { + if (!hasShowMoreBtn.value || showAll.value) { return [] } return props.items?.slice(hiddenIndex.value) || [] })Likely an incorrect or invalid review comment.
…components - Changed item IDs in `pills-popper-config.vue` to start from 2 for better uniqueness. - Refactored visibility logic in `index.vue` to simplify conditions for showing static and floating items based on button visibility and state. - Removed unnecessary gap calculation function and replaced it with a fixed gap value for consistency.
…ity handling - Introduced a button in `pills-popper-config.vue` to allow users to dynamically add items to the suggestion list. - Implemented a `closeAllPopper` function to streamline the visibility management of suggestion items. - Enhanced click handling to ensure proper visibility toggling when interacting with the new button.
- Updated gap calculation in `index.vue` to dynamically retrieve the row gap from the container's computed style instead of using a fixed value. - Added a check to ensure the container reference is valid before performing width calculations on item elements.
2 participants
onClickOutsideAPI 的代码，使用内置的ignore来忽略点击元素SuggestionPills配置式时，将events下的属性全部迁移到props，使用onXXX来传入事件回调SuggestionPills最大宽度的逻辑，保证动态添加按钮后，最大宽度更新正确Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation