feat: improve ApiSelect component
#5075
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web-antd/src/adapter/component/index.tsOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request introduces a new component type, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (8)packages/effects/common-ui/src/components/api-select/api-select.vue (3)
In the Please ensure that the function correctly handles scenarios where
The use of
Critical Issue: Default 'childrenField' value may cause unexpected behavior Setting It's recommended to set a sensible default value, such as Suggested fix: - childrenField: '',
+ childrenField: 'children',playground/src/adapter/component/index.ts (3)
Including
The change from
The addition of the apps/web-ele/src/adapter/component/index.ts (2)
The Ensure that the event name matches the actual event emitted by
The implementation of 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 (5)
apps/web-naive/src/views/demos/form/basic.vue (1)
30-34: Refine TypeScript typing inafterFetchfunctionIn the
afterFetchfunction, the parameteritemis currently typed asany. Sincedatais of type{ name: string; path: string }[], it would be beneficial to specify the type ofitemfor better type safety and IntelliSense support.Suggested change:
- return data.map((item: any) => ({ + return data.map((item: { name: string; path: string }) => ({ label: item.name, value: item.path, }))apps/web-ele/src/views/demos/form/basic.vue (3)
31-36: Improve type safety in theafterFetchtransformerThe current implementation uses
anytype which reduces type safety. Consider using a proper interface for the menu data structure.- afterFetch: (data: { name: string; path: string }[]) => { - return data.map((item: any) => ({ + afterFetch: (data: Array<{ name: string; path: string }>) => { + return data.map((item) => ({ label: item.name, value: item.path, })); },
46-62: Add JSDoc comments for the ApiTreeSelect configurationThe field mappings are crucial for the tree structure. Consider adding documentation to explain the expected data structure and field mappings.
{ + /** + * @description Tree select component that maps API data to a hierarchical structure + * @field childrenField - Field name for nested items (default: 'children') + * @field labelField - Field name for display text (default: 'name') + * @field valueField - Field name for selection value (default: 'path') + */ component: 'ApiTreeSelect', componentProps: { api: getAllMenusApi,
Line range hint
171-183: Update setFormValues to include new API-based fieldsThe
setFormValuesfunction should be updated to include initial values for the newapiandapiTreefields for consistency.function setFormValues() { formApi.setValues({ + api: null, // or default value + apiTree: null, // or default value string: 'string', number: 123, radio: 'B',playground/src/views/examples/form/basic.vue (1)
65-81: LGTM! Consider adding validation and type safety.The
ApiTreeSelectimplementation follows good practices and integrates well with the existing form structure. A few suggestions for enhancement:
- Consider adding validation rules for required/optional status
- Add TypeScript interface for the API response structure
- Consider handling loading states for better UX
Example enhancement:
{ component: 'ApiTreeSelect', componentProps: { api: getAllMenusApi, childrenField: 'children', labelField: 'name', placeholder: '请选择', valueField: 'path', + // Add loading state handling + fieldNames: { + label: 'name', + value: 'path', + children: 'children' + }, }, fieldName: 'apiTree', label: 'ApiTreeSelect', + // Add validation rules + rules: [{ required: true, message: '请选择选项' }], },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/web-antd/src/adapter/component/index.ts(2 hunks)apps/web-ele/src/adapter/component/index.ts(2 hunks)apps/web-ele/src/views/demos/form/basic.vue(2 hunks)apps/web-naive/src/adapter/component/index.ts(2 hunks)apps/web-naive/src/views/demos/form/basic.vue(2 hunks)packages/effects/common-ui/src/components/api-select/api-select.vue(3 hunks)playground/src/adapter/component/index.ts(2 hunks)playground/src/views/examples/form/basic.vue(1 hunks)
🔇 Additional comments (6)
playground/src/adapter/component/index.ts (1)
98-113: Verify compatibility of fieldNames with ApiSelect component
In the ApiTreeSelect implementation, the fieldNames prop is provided. Please ensure that the ApiSelect component accepts and properly handles the fieldNames prop when used with TreeSelect. This is to guarantee that the label, value, and children fields are correctly mapped, and the nested data structures function as intended.
apps/web-antd/src/adapter/component/index.ts (1)
97-112:
Confirm event handler name for dropdown visibility
In the ApiTreeSelect component, the visibleEvent is set to 'onVisibleChange', whereas in the ApiSelect component for Ant Design Vue, the correct event for dropdown visibility might be 'onDropdownVisibleChange'. Please verify that the event name matches the actual event emitted by the TreeSelect component to ensure that data fetching is triggered appropriately when the dropdown is opened.
Suggested change:
- visibleEvent: 'onVisibleChange',
+ visibleEvent: 'onDropdownVisibleChange',apps/web-naive/src/adapter/component/index.ts (1)
83-99: LGTM! Clean implementation of ApiTreeSelect adapter
The implementation correctly maps the ApiSelect component to Naive UI's TreeSelect with appropriate prop mappings and event handling.
apps/web-ele/src/adapter/component/index.ts (2)
86-99: LGTM! Well-structured ApiTreeSelect adapter
The implementation correctly maps to Element Plus's TreeSelect component with appropriate prop mappings for label, children, and model value.
81-85: Verify the visibleEvent change impact
The change from 'onDropdownVisibleChange' to 'onVisibleChange' affects the ApiSelect component. Ensure this change is compatible with Element Plus's latest API.
✅ Verification successful
Let's examine the specific usage of these events across different adapters to understand the implementation patterns.
Let's verify the Element Plus API documentation to confirm the correct event name.
Based on the analysis of the codebase, I can now provide the final response:
The visibleEvent change is consistent with UI framework patterns
The change to 'onVisibleChange' in Element Plus adapter aligns with the event naming patterns across other UI framework adapters:
- Naive UI uses 'onVisibleChange'
- Element Plus uses 'onVisibleChange'
- Only Ant Design has a different pattern using 'onDropdownVisibleChange' for Select but 'onVisibleChange' for TreeSelect
The change maintains consistency with Element Plus's component event naming conventions and other framework adapters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other usages of onDropdownVisibleChange or onVisibleChange
rg -l "onDropdownVisibleChange|onVisibleChange" --type ts
Length of output: 236
Script:
#!/bin/bash
# Check the context of these event usages across adapters
rg "onDropdownVisibleChange|onVisibleChange" --type ts -C 5
Length of output: 5448
Script:
#!/bin/bash
# Search for the Element Plus component definition to verify the event name
rg "ElSelectV2" --type ts -C 5
Length of output: 1380
playground/src/views/examples/form/basic.vue (1)
69-76: Verify API response structure compatibility
The component uses the same API (getAllMenusApi) as ApiSelect but with different field mappings. Let's verify the API response structure matches both use cases.
✅ Verification successful
Let me analyze the results:
-
The
getAllMenusApiis consistently used across multiple applications (playground, web-naive, web-ele, web-antd) and returnsRouteRecordStringComponent[]type. -
The field mappings in the form component:
childrenField: 'children'labelField: 'name'valueField: 'path'
-
The ApiSelect component (in packages/effects/common-ui/src/components/api-select/api-select.vue) has default mappings:
labelField: 'label'valueField: 'value'childrenField: ''
-
However, these default mappings can be overridden through props, and the component properly handles custom field mappings through the
transformDatafunction.
Let me generate the final response:
Field mappings are correctly configured
The component's field mappings (name, path, children) are compatible with the API response structure. While ApiSelect has different default mappings, it's designed to handle custom field names through props, which are properly transformed in its implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API usage consistency and response structure
# Search for getAllMenusApi usage patterns
echo "Searching for getAllMenusApi usage patterns..."
rg -A 5 "getAllMenusApi"
# Search for field name usage consistency
echo "Searching for field name patterns..."
rg -A 2 "labelField|valueField|childrenField"
Length of output: 13010
Description
改进ApiSelect组件，使之可以支持更多类型的组件包装。例子中提供了TreeSelect组件的Api封装。
close #5074
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
ApiTreeSelectcomponent for enhanced hierarchical data selection.ApiSelectandApiTreeSelectcomponents for dynamic API data integration.Bug Fixes
ApiSelect.Documentation
Chores
api-selectcomponent for better API interaction and data handling.