fix(select): [select] fix the problem of multiple selection of hover … #2566
Conversation
WalkthroughThe changes in this pull request enhance the functionality of the select component across multiple files. A new computed property, Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@MomoPoppy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/renderless/src/select/vue.ts (2)
Line range hint
1-113: API array is missing some of the new computed properties.The API array should include all the computed properties that are part of the component's public interface. Currently, only
isTagClosableis included, butcomputedGetIcon,computedGetTagType, andcomputedCurrentSizeMapare missing.Add the missing computed properties to the API array:
export const api = [ // ... existing entries ... 'getLabelSlotValue', 'loadTreeData', 'updateModelValue', 'clearSearchText', 'onClickCollapseTag', 'computedShowTagText', - 'isTagClosable' + 'isTagClosable', + 'computedGetIcon', + 'computedGetTagType', + 'computedCurrentSizeMap' ]
Line range hint
1-644: Good architectural approach with renderless pattern.The implementation follows the renderless component pattern, which promotes good separation of concerns between logic and presentation. The new functionality for icons and tag types is integrated consistently with this pattern.
However, consider the following architectural improvements:
- Document the purpose and expected values of the new computed properties
- Consider extracting the icon and tag type logic into a separate composable if they might be reused in other components
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/renderless/src/select/index.ts(2 hunks)packages/renderless/src/select/vue.ts(3 hunks)packages/theme/src/select/index.less(1 hunks)packages/vue/src/select/src/pc.vue(1 hunks)
🔇 Additional comments (5)
packages/theme/src/select/index.less (1)
225-230: LGTM! Please verify the fix and add documentation.
The addition of vertical alignment for display-only multiple select in hover/click expand states looks correct. However:
- Please verify that this change fully addresses the multiple selection hover issue mentioned in the PR objectives.
- Consider adding a comment explaining why this vertical alignment is necessary to help future maintainers.
Let's verify if this style change affects other components or states:
✅ Verification successful
Vertical alignment change is consistent with existing patterns
The vertical-align: top property is already used consistently across related components:
- It's applied to hover/click expand states in base-select
- It's used in cascader's hover-expand state
- The new addition for multiple select display-only follows the same pattern
The change is safe and aligns with the existing codebase conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by vertical alignment changes
# or that use similar expand states
# Look for other components using similar class combinations
rg -l "is-hover-expand|is-click-expand" --type less
# Look for potential vertical alignment dependencies
rg -l "vertical-align.*top.*select" --type less
Length of output: 307
Script:
#!/bin/bash
# Let's examine the related files to understand the impact and consistency
# Check how vertical-align is used in base-select
rg "vertical-align" packages/theme/src/base-select/index.less -B 2 -A 2
# Check the context of hover/click expand in cascader
rg "is-hover-expand|is-click-expand" packages/theme/src/cascader/index.less -B 2 -A 2
# Look for similar multiple selection display-only patterns
rg "multiple.*display-only" packages/theme/src/base-select/index.less packages/theme/src/select/index.less -B 2 -A 2
Length of output: 870
packages/renderless/src/select/vue.ts (2)
247-248: LGTM: State initialization is well-structured.
The new state properties are properly initialized with computed values and the designConfig integration is handled correctly with proper fallbacks.
409-410: Verify the implementation of isTagClosable.
The isTagClosable function is called without parameters during API initialization, but it might need access to component state or props to determine if a tag is closable.
Let's check the implementation of isTagClosable:
✅ Verification successful
The isTagClosable implementation is correct
The implementation of isTagClosable is a higher-order function that returns a function taking an item parameter, which is exactly what's needed. The function is defined in both packages/renderless/src/select/index.ts and packages/renderless/src/base-select/index.ts as:
export const isTagClosable = () => (item) => !item.requiredThis matches its usage in the Vue templates where it's called with an item parameter:
:closable="isTagClosable(item)"
:closable="isTagClosable(state.selected[0])"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of isTagClosable
ast-grep --pattern 'isTagClosable = $_' -l ts,js
Length of output: 161
Script:
#!/bin/bash
# Search for the implementation of isTagClosable with correct language flags
ast-grep --pattern 'isTagClosable = $_'
# Also search for any declaration or definition using ripgrep
rg "isTagClosable" -A 3
Length of output: 4074
packages/renderless/src/select/index.ts (2)
2051-2058: LGTM! Well-structured computed property for size management.
The implementation provides good flexibility with design system configuration while maintaining sensible defaults.
752-752: LGTM! Good use of computed property for dynamic sizing.
The changes properly integrate the computed size map and handle spacing configuration, making the component more maintainable and configurable.
Also applies to: 756-756
| @@ -151,7 +151,7 @@ | |||
| :type="state.getTagType" | |||
| key="tags-collapse" | |||
| data-tag="tags-collapse" | |||
| only-icon | |||
| :only-icon="!hoverExpand" | |||
There was a problem hiding this comment.
Ensure hoverExpand prop is defined to prevent runtime errors
The hoverExpand property is used in the template but is not currently defined as a prop or within the component's data or computed properties. This could lead to runtime errors due to accessing an undefined property.
Apply this diff to define hoverExpand as a prop:
props: [
...props,
// 以下为 tiny 新增
'searchable',
'showEmptyImage',
'inputBoxType',
'tagType',
'clearNoMatchValue',
'showLimitText',
'showProportion',
'clickExpand',
'maxVisibleRows',
'showAllTextTag',
'allText',
+ 'hoverExpand'
],Committable suggestion skipped: line range outside the PR's diff.
…Expand not being centered
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
computedCurrentSizeMap.searchable,showEmptyImage,inputBoxType,tagType,clearNoMatchValue,showLimitText,showProportion,clickExpand,maxVisibleRows,showAllTextTag, andallText.Bug Fixes
Style Updates
These enhancements aim to improve usability, flexibility, and responsiveness of the select component.