fix(tree-select): add emit modelValue #2461
Conversation
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
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 (2)
packages/renderless/src/tree-select/vue.ts (1)
5-20: Consider adding JSDoc documentation
Since this is a renderless component implementation, consider adding JSDoc documentation to describe:
- The component's purpose
- The expected props interface
- The emitted events (especially the new modelValue emit)
- The returned API shape
This would improve maintainability and make it easier for other developers to use the component correctly.
packages/renderless/src/tree-select/index.ts (1)
31-46: LGTM! Consider adding type safety
The implementation correctly collects and emits multiple selected values. The event emissions follow Vue's v-model convention and are properly placed after data processing.
Consider adding type safety to improve maintainability:
- const currentValue = []
+ const currentValue: Array<string | number> = []📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/renderless/src/tree-select/index.ts (1 hunks)
- packages/renderless/src/tree-select/vue.ts (1 hunks)
🔇 Additional comments (5)
packages/renderless/src/tree-select/vue.ts (3)
5-5: LGTM: Function signature properly updated for v-model support
The addition of computed and emit parameters aligns with Vue's v-model implementation pattern, enabling proper two-way binding support.
9-9: LGTM: Reactive value properly implemented
Converting value to a computed property based on modelValue ensures proper reactivity and follows Vue's best practices for v-model implementation.
17-17: Verify emit implementation in check function
The emit parameter is now correctly passed to the check function, but we should verify its implementation.
✅ Verification successful
check function implementation verified and working correctly
The check function correctly uses the emit parameter to update the model value:
- It emits 'change' and 'update:modelValue' events with the current value
- Properly handles multiple selection mode
- Correctly maps the checked nodes using the configured value and text fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the check function implementation in index.ts
# Expected: check function should use emit parameter for modelValue updates
# Search for check function implementation
ast-grep --pattern 'export const check = ({ props, vm, emit }) => {
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Let's try to find the check function implementation with a broader search
rg "check = " -A 10
Length of output: 2492
Script:
#!/bin/bash
# Also search for any file that might contain the check function
fd "check.ts"
Length of output: 13
Script:
#!/bin/bash
# Search in the specific directory
rg "check = " packages/renderless/src/tree-select/ -A 10
Length of output: 58
Script:
#!/bin/bash
# Look for the function with a different pattern
ast-grep --pattern 'function check({ $$ }) {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Let's try to find the check function in the tree-select directory with a different approach
rg "check\(" packages/renderless/src/tree-select/ -A 10
Length of output: 331
Script:
#!/bin/bash
# Also look for files in the tree-select directory
fd . packages/renderless/src/tree-select/
Length of output: 131
Script:
#!/bin/bash
# Try to find any exports from index.ts in tree-select
rg "export" packages/renderless/src/tree-select/
Length of output: 475
Script:
#!/bin/bash
# Let's look at the content of index.ts to see the check function implementation
cat packages/renderless/src/tree-select/index.ts
Length of output: 1146
packages/renderless/src/tree-select/index.ts (2)
28-28: LGTM! Proper function signature update
The addition of the emit parameter to the function signature is correct and follows Vue's composition API patterns.
20-46: Verify v-model integration in parent components
The implementation of modelValue emissions is correct and follows Vue's v-model convention. However, ensure that parent components using this tree-select component have been updated to use v-model properly.
Let's verify the usage:
✅ Verification successful
Let's verify the v-model usage in these components to ensure proper integration.
V-model integration is properly implemented and used
The verification shows that the v-model implementation is correctly used across the codebase:
- Parent components properly use v-model directive with the tree-select component
- Both single and multiple selection modes demonstrate correct v-model usage
- The internal implementation in pc.vue correctly binds to the state value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tree-select usage in parent components
rg -l "tree-select" | rg "\.vue$" | xargs rg -l "v-model"
Length of output: 1355
Script:
#!/bin/bash
# Check v-model usage in tree-select components
rg -A 2 -B 2 "v-model" examples/sites/demos/pc/app/tree-select/basic-usage.vue examples/sites/demos/pc/app/tree-select/multiple.vue packages/vue/src/tree-select/src/pc.vue
Length of output: 1151
| emit('change', data[props.valueField]) | ||
| emit('update:modelValue', data[props.valueField]) | ||
|
|
There was a problem hiding this comment.
Add missing emit parameter to nodeClick function signature
The event emissions are correct, but the emit parameter needs to be added to the function signature to match its usage.
Apply this diff to fix the function signature:
-export const nodeClick = ({ props, vm }) =>
+export const nodeClick = ({ props, vm, emit }) =>Committable suggestion was skipped due to low confidence.
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
New Features
Bug Fixes
Documentation