Momo poppy/fix select 1126 #2565
Conversation
WalkthroughThe pull request introduces several modifications across multiple files related to the select and option components. Key changes include the addition of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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 (9)
packages/vue/src/option/src/pc.vue (1)
4-4: Consider adding keyboard navigation support.While adding mouse hover functionality, ensure the component remains accessible via keyboard navigation. Consider implementing keyboard event handlers (arrow keys) for option navigation.
Example implementation:
<li ref="option" @mouseenter="hoverItem" + @keydown.up.prevent="navigateOptions('prev')" + @keydown.down.prevent="navigateOptions('next')" + @keydown.enter.prevent="selectOptionClick" @click.stop="selectOptionClick"examples/sites/demos/pc/app/select/multiple.spec.ts (1)
45-47: LGTM! Consider adding hover state transition tests.The updated assertions correctly verify the component's behavior when selection limit is reached:
- First option shows both selected and required states
- Third option shows both selected and hover states
Consider adding test cases to verify hover state transitions when moving between options. For example:
// Verify hover state transitions await option.nth(0).hover(); await expect(list[0]).toHaveClass(/selected is-required hover/); await expect(list[2]).toHaveClass(/selected/); await option.nth(2).hover(); await expect(list[0]).toHaveClass(/selected is-required/); await expect(list[2]).toHaveClass(/selected hover/);packages/vue/src/option/src/mobile-first.vue (2)
Line range hint
4-24: Consider mobile-specific hover behaviorThe component uses the
@mouseenterevent which might not be optimal for mobile devices. The hover state (viastate.hover) is already being handled in the class bindings, but mobile devices typically don't have true hover capability.Consider:
- Using touch-specific events for mobile devices
- Implementing a responsive event handling strategy that adapts based on the device type
- Adding a comment explaining the hover state management strategy for both mobile and desktop contexts
Line range hint
11-24: Enhance accessibility for hover statesWhile the visual hover feedback is well-implemented, consider adding ARIA attributes to improve accessibility:
:class=" m( 'h-fit flex items-center justify-between rounded text-color-text-primary text-sm sm:text-xs', 'min-h-[40px] sm:min-h-[32px] cursor-pointer group sm:hover:bg-color-bg-4 sm:active:bg-color-bg-3 pl-0 pr-3 sm:px-3 my-1 sm:m-1', { 'text-color-brand bg-color-bg-1 sm:bg-color-fill-6': state.itemSelected && !disabled, 'text-color-text-disabled cursor-not-allowed [&_svg]:fill-color-icon-disabled [&_svg_path:first-of-type]:fill-color-bg-3': disabled || state.groupDisabled || state.limitReached, 'bg-color-bg-1 sm:bg-color-bg-4': state.hover, 'text-color-text-disabled cursor-not-allowed': required }, highlightClass ) " + :aria-selected="state.itemSelected" + :aria-disabled="disabled || state.groupDisabled || state.limitReached" data-tag="tiny-option"packages/renderless/src/option/index.ts (1)
52-58: Add null checks and handle edge cases.The implementation looks good overall, but could benefit from additional safety checks:
export const hoverItem = ({ select, props, state }) => () => { - if (!props.disabled && !state.groupDisabled && !select.state.disabledOptionHover) { + if (!props?.disabled && !state?.groupDisabled && select?.state && !select.state.disabledOptionHover) { + const index = select.state.optionIndexArr?.indexOf(state.index) ?? -1 + if (index !== -1) { - select.state.hoverIndex = select.state.optionIndexArr.indexOf(state.index) + select.state.hoverIndex = index + } } }packages/renderless/src/option/vue.ts (1)
71-71: Consider adding tests for the hover functionalityWhile the implementation looks solid, it would be beneficial to add test cases covering:
- Hover state changes
- Interaction with disabled states
- Edge cases in multi-select scenarios
Would you like me to help generate test cases for the new hover functionality?
packages/theme/src/select/index.less (1)
225-230: LGTM! Consider adding a comment to document the alignment behavior.The new style rule correctly handles the vertical alignment for display-only multiple select when expanded.
Consider adding a comment to explain why this specific vertical alignment is needed for display-only mode:
+ // Ensures proper alignment of expanded display-only multiple select to prevent layout shifts &.tiny-select__multiple.is-display-only { &.is-hover-expand, &.is-click-expand { vertical-align: top; } }packages/renderless/src/select/vue.ts (1)
247-248: Avoid adding non-reactive properties directly to the reactive stateAssigning
designConfigdirectly to thestateobject may introduce non-reactive properties, which can lead to unexpected behavior within the Vue reactivity system.Consider excluding
designConfigfrom the reactivestateobject. IfdesignConfigneeds to be accessed withinstate, usetoRefsor compute it as a reactive property.Apply this diff to adjust the assignment:
- designConfig,Alternatively, if
designConfigmust be reactive:+ designConfig: reactive(designConfig),packages/renderless/src/select/index.ts (1)
2051-2059: Validatestate.selectSizeto prevent accessing undefined size valuesCurrently,
sizeMap[state.selectSize || 'default']may returnundefinedifstate.selectSizeis not a key insizeMap. Consider adding validation to ensurestate.selectSizeis one of the expected keys, or provide a default value to prevent potential issues.Apply this diff to add validation:
export const computedCurrentSizeMap = ({ state, designConfig }) => () => { const defaultSizeMap = { default: 32, mini: 24, small: 28, medium: 40 } const sizeMap = designConfig?.state?.sizeMap || defaultSizeMap + const sizeKey = sizeMap[state.selectSize] ? state.selectSize : 'default' - return sizeMap[state.selectSize || 'default'] + return sizeMap[sizeKey] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
examples/sites/demos/pc/app/select/multiple.spec.ts(1 hunks)packages/renderless/src/option/index.ts(1 hunks)packages/renderless/src/option/vue.ts(2 hunks)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/option/src/mobile-first.vue(1 hunks)packages/vue/src/option/src/pc.vue(1 hunks)packages/vue/src/select/src/pc.vue(1 hunks)
🔇 Additional comments (9)
packages/vue/src/option/src/pc.vue (1)
4-4: LGTM! Verify hoverItem implementation.
The addition of the mouseenter event handler aligns well with the component's existing mouse event handling pattern.
Let's verify the hoverItem implementation:
✅ Verification successful
The mouseenter event handler is correctly implemented and used consistently
The verification confirms:
hoverItemis properly defined in the renderless API (packages/renderless/src/option/index.ts)- It handles disabled states appropriately and updates the select component's hover index
- The handler is consistently used across both PC and mobile-first components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hoverItem implementation in the renderless API
# Expected: Find the hoverItem function definition and its usage
# Search for hoverItem definition in renderless files
rg -A 5 "hoverItem.*=" "packages/renderless/src/option/"
# Search for other usages to ensure consistency
rg "hoverItem" "packages/vue/src/option/"
Length of output: 714
packages/vue/src/option/src/mobile-first.vue (1)
4-4: Verify the hoverItem implementation in renderless API
The addition of the mouseenter event handler appears to be part of a larger hover state management enhancement. However, we should verify that the hoverItem method is properly implemented in the renderless API.
✅ Verification successful
The hoverItem method is properly implemented and integrated
The verification shows that:
- The
hoverItemmethod is properly implemented in the renderless API (packages/renderless/src/option/index.ts) - It's correctly exported in the API list (
packages/renderless/src/option/vue.ts) - The method is properly integrated with hover state management in the mobile-first component
- The hover state is used for styling with appropriate hover effects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hoverItem implementation in the renderless API
# Check for hoverItem in the renderless implementation
ast-grep --pattern 'hoverItem($$$) {
$$$
}' packages/renderless/src/option/index.ts
# Check if hoverItem is exported in the API
ast-grep --pattern 'export const api = [
$$$
"hoverItem",
$$$
]' packages/renderless/src/option/vue.ts
Length of output: 198
Script:
#!/bin/bash
# Let's try a broader search to find any references to hoverItem
# Search for hoverItem in the entire codebase
rg -l "hoverItem" --type ts --type vue
# Search for hover-related methods in option-related files
rg -l "hover" packages/renderless/src/option/
# Check the actual implementation file content
fd "index.ts" packages/renderless/src/option/ -x cat {}
# Look for any hover-related methods in the Vue implementation
fd "mobile-first.vue" -x cat {}
Length of output: 101000
Script:
#!/bin/bash
# Let's search for hoverItem method in the option component and related files
# Search for hoverItem in option-related files
rg -A 5 "hoverItem" packages/renderless/src/option/
# Check if hoverItem is used in the mobile-first.vue component
rg -A 5 "hoverItem" packages/vue/src/option/src/mobile-first.vue
# Look for any hover-related state management in the option component
rg "hover" packages/vue/src/option/src/mobile-first.vue
Length of output: 2479
packages/renderless/src/option/index.ts (1)
52-58: Consider caching the option index for better performance.
For large lists, repeatedly calling indexOf on mouseenter events could impact performance. Consider storing the index in the state during initialization.
Let's check if there are any large option lists in the codebase:
packages/renderless/src/option/vue.ts (3)
13-22: LGTM: Import statement changes are well-structured
The addition of hoverItem to the imports maintains consistency with the existing pattern and groups related functionality together.
71-71: Verify the hoverItem implementation and its integration
The hoverItem function is properly integrated with its dependencies. However, we should verify:
- The implementation in the index file
- The interaction with the select component's state
- Usage patterns in consuming components
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the implementation and integration of hoverItem.
The hoverItem implementation is correctly integrated and used
The implementation is properly structured and follows the expected patterns:
- The function is correctly exported and implemented in
packages/renderless/src/option/index.ts - It's properly integrated in both PC and Mobile-first option components via mouseenter events
- The state interactions are correctly handled through
select.state.hoverIndexandselect.state.hoverValue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation in index.ts
ast-grep --pattern 'export const hoverItem = $_'
# Check usage in components
rg "hoverItem" -A 3 -g "*.vue"
# Check select state interactions
ast-grep --pattern 'select.state.hoverValue'
Length of output: 1360
24-24: Verify API documentation for the new hoverItem function
The hoverItem function has been added to the public API. Please ensure that this new API method is properly documented in the component's API documentation.
packages/renderless/src/select/vue.ts (1)
112-113: Verify the existence and export of new computed properties
Please ensure that isTagClosable and computedCurrentSizeMap are correctly defined and exported in ./index. This verification is essential to prevent potential import errors.
Run the following script to check if these functions are exported:
✅ Verification successful
Both properties are correctly defined and exported
Both isTagClosable and computedCurrentSizeMap are properly defined and exported in packages/renderless/src/select/index.ts. The isTagClosable is a function that checks if an item is not required, and computedCurrentSizeMap is a computed property that returns size values based on the select size configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'isTagClosable' and 'computedCurrentSizeMap' are exported from './index.js'
# Expected: Declarations of 'export const isTagClosable' and 'export const computedCurrentSizeMap' should be found
ast-grep --pattern $'export const isTagClosable = $_' ./index.js
ast-grep --pattern $'export const computedCurrentSizeMap = $_' ./index.js
Length of output: 256
Script:
#!/bin/bash
# First, let's find the correct path to the index file in the select directory
fd "index.ts" packages/renderless/src/select
# Then, let's check for these exports in any file within the select directory
rg "export const (isTagClosable|computedCurrentSizeMap)" packages/renderless/src/select -l
# Let's also check the actual implementation of these properties
ast-grep --pattern $'const isTagClosable = $_'
ast-grep --pattern $'const computedCurrentSizeMap = $_'
Length of output: 1174
packages/vue/src/select/src/pc.vue (1)
154-154: Dynamic binding of only-icon to match hover state
The change to bind :only-icon dynamically with !hoverExpand ensures that the tiny-tag component displays the icon appropriately based on the hover state, enhancing user interaction.
packages/renderless/src/select/index.ts (1)
752-756: Calculation of input.style.height uses dynamic size mapping
The use of state.currentSizeMap in calculating input.style.height ensures consistency with dynamic size configurations, enhancing adaptability to different themes and sizes.
| isTagClosable: isTagClosable(), | ||
| computedCurrentSizeMap: computedCurrentSizeMap({ state, designConfig }) |
There was a problem hiding this comment.
Correct the assignment of functions in initApi
The function isTagClosable is being invoked immediately instead of being assigned as a function reference. This may lead to unintended execution during initialization.
Modify the assignment to reference the function without invoking it:
- isTagClosable(),
+ isTagClosable: isTagClosable,Similarly, ensure that computedCurrentSizeMap is properly assigned:
- computedCurrentSizeMap: computedCurrentSizeMap({ state, designConfig })
+ computedCurrentSizeMap: (args) => computedCurrentSizeMap({ state, designConfig, ...args }),This ensures that the functions are correctly included in the api object without unintended side effects.
Committable suggestion skipped: line range outside the PR's diff.
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
hoverItemfunction to enhance hover interactions in the select component.computedGetIcon,computedGetTagType, andcomputedCurrentSizeMap.searchable,showEmptyImage, and more.Bug Fixes
Style
Documentation