refactor: ApiComponent with docs
#5099
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
|
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 changes in this pull request involve the renaming and replacement of the 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 (
|
mynetfan
force-pushed
the
refactor/api-component
branch
from
47d57e3 to
1a170c2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
docs/src/demos/vben-api-component/cascader/index.vue (1)
83-89: Consider enhancing the mock API implementationThe mock API implementation is functional but could benefit from error handling and loading state simulation.
Consider this enhanced implementation:
function fetchApi(): Promise<Record<string, any>> { return new Promise((resolve) => { setTimeout(() => { + // Simulate API success rate + if (Math.random() > 0.9) { + throw new Error('Simulated API failure'); + } resolve(treeData); }, 1000); }); }playground/src/adapter/component/index.ts (1)
Line range hint
86-95: Verify ApiComponent configuration consistencyThe ApiSelect implementation uses
onVisibleChangewhile the demo usesonDropdownVisibleChange. This inconsistency might cause issues.Apply this fix:
ApiComponent, { placeholder: $t('ui.placeholder.select'), ...props, ...attrs, component: Select, loadingSlot: 'suffixIcon', modelPropName: 'value', - visibleEvent: 'onVisibleChange', + visibleEvent: 'onDropdownVisibleChange', },docs/src/components/common-ui/vben-api-component.md (3)
148-149: Remove empty code fence block.The empty code fence block at the end of the file should be removed as it serves no purpose.
-``` - -```🧰 Tools
🪛 Markdownlint (0.35.0)
148-148: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
136-136: Enhance childrenField prop description.The description for
childrenFieldprop shows an empty default value. Consider adding more context about when this field should be used and its importance in hierarchical data structures.-| childrenField | 子级数据字段名，需要层级数据的组件可用 | `string` | `` | +| childrenField | 子级数据字段名，用于树形结构数据（如TreeSelect、Cascader等）中指定子节点的字段名 | `string` | `` |
21-122: Enhance example code documentation.While the example demonstrates Cascader integration well, it could benefit from additional comments explaining key configuration options and data structure requirements.
Add explanatory comments for key props:
<template> <ApiComponent :api="fetchApi" :component="Cascader" + <!-- Disable immediate loading to fetch data only when dropdown opens --> :immediate="false" + <!-- Specify the field name for nested children in the tree structure --> children-field="children" + <!-- Configure loading indicator to appear in the suffix icon position --> loading-slot="suffixIcon" + <!-- Event that triggers data fetching when dropdown visibility changes --> visible-event="onDropdownVisibleChange" /> </template>apps/web-naive/src/adapter/component/index.ts (1)
Based on the search results from the previous commands, I can now provide a final response:
TreeSelect prop mappings need adjustment for Naive UI
The current configuration appears to mix prop names from different UI frameworks. For Naive UI's TreeSelect:
nodeKeyandkeyFieldare redundant - Naive UI uses onlykeyFieldfor unique node identificationloadingSlotshould be 'arrow' as correctly set (verified from other adapter implementations)🔗 Analysis chain
Line range hint
86-97: Verify TreeSelect specific prop mappings.The TreeSelect configuration includes additional props specific to tree structures. Please verify that these prop names match Naive UI's TreeSelect component API:
nodeKeykeyFieldloadingSlot
Let me gather more information about NTreeSelect's API specifically.
Let me try a different approach to find TreeSelect related information.
Let me try one more search to find Naive UI's TreeSelect documentation or type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TreeSelect usage in the codebase to verify prop names rg -A 5 "NTreeSelect.*nodeKey|keyField|loadingSlot"Length of output: 5219
Script:
#!/bin/bash # Search for NTreeSelect documentation or type definitions rg -A 10 "NTreeSelect|TreeSelect.*props" --type typescriptLength of output: 95
Script:
#!/bin/bash # Search for TreeSelect related files and their content rg -A 10 "TreeSelect"Length of output: 29556
Script:
#!/bin/bash # Search for NTreeSelect props or documentation rg "NTreeSelect.*props|interface.*TreeSelect" -A 5Length of output: 52
packages/effects/common-ui/src/components/api-component/api-component.vue (1)
Line range hint
133-146: Enhance error handling in fetchApi function.The current error handling only logs to console and resets the loading state. Consider adding error state management and error event emission.
} catch (error) { console.warn(error); // reset status isFirstLoaded.value = false; + emit('error', error); + refOptions.value = props.options || []; }Also, consider adding error event to the emits definition:
const emit = defineEmits<{ optionsChange: [OptionsItem[]]; + error: [Error]; }>();apps/web-ele/src/adapter/component/index.ts (2)
12-12: Consider updating ComponentType to reflect the new ApiComponent architectureWhile the implementation has been updated to use ApiComponent, the ComponentType still lists 'ApiSelect' and 'ApiTreeSelect' as separate types. Consider adding 'ApiComponent' to maintain consistency with the new architecture.
export type ComponentType = + | 'ApiComponent' | 'ApiSelect' | 'ApiTreeSelect' | 'Checkbox' // ... rest of the typesAlso applies to: 32-52
Line range hint
73-97: Consider extracting common ApiComponent configurationBoth ApiSelect and ApiTreeSelect share similar configuration. Consider extracting the common props to reduce duplication:
+const commonApiProps = { + placeholder: $t('ui.placeholder.select'), + loadingSlot: 'loading', + visibleEvent: 'onVisibleChange', +}; ApiSelect: (props, { attrs, slots }) => { return h( ApiComponent, { - placeholder: $t('ui.placeholder.select'), + ...commonApiProps, ...props, ...attrs, component: ElSelectV2, - loadingSlot: 'loading', - visibleEvent: 'onVisibleChange', }, slots, ); },docs/.vitepress/config/zh.mts (1)
165-168: LGTM: Documentation structure is well-organizedThe ApiComponent documentation is properly integrated into the sidebar under the Common Components section. Consider adding a brief description in the sidebar text to better indicate its wrapper functionality.
{ link: 'common-ui/vben-api-component', - text: 'ApiComponent Api组件包装器', + text: 'ApiComponent 通用API数据加载组件包装器', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
apps/web-antd/src/adapter/component/index.ts(3 hunks)apps/web-ele/src/adapter/component/index.ts(3 hunks)apps/web-naive/src/adapter/component/index.ts(3 hunks)docs/.vitepress/config/zh.mts(1 hunks)docs/src/components/common-ui/vben-api-component.md(1 hunks)docs/src/demos/vben-api-component/cascader/index.vue(1 hunks)packages/effects/common-ui/src/components/api-component/api-component.vue(3 hunks)packages/effects/common-ui/src/components/api-component/index.ts(1 hunks)packages/effects/common-ui/src/components/api-select/index.ts(0 hunks)packages/effects/common-ui/src/components/index.ts(1 hunks)playground/src/adapter/component/index.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/effects/common-ui/src/components/api-select/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/effects/common-ui/src/components/index.ts
- packages/effects/common-ui/src/components/api-component/index.ts
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/src/components/common-ui/vben-api-component.md
148-148: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
docs/src/demos/vben-api-component/cascader/index.vue (1)
92-99: LGTM! Well-configured ApiComponent integration
The ApiComponent is properly configured with Cascader, demonstrating key features:
- Lazy loading through
immediate: false - Correct field mapping with
children-field - Loading state handling via
loading-slot - Proper event handling with
visible-event
playground/src/adapter/component/index.ts (1)
Line range hint 101-112: Consider standardizing field name configurations
The ApiTreeSelect implementation explicitly defines fieldNames while the base Select doesn't. Consider standardizing this approach across all select-like components.
Let's verify if other select-like components need similar field mappings:
apps/web-antd/src/adapter/component/index.ts (2)
Line range hint 85-94: Fix visibleEvent prop for consistency
The same inconsistency with visibleEvent exists in this implementation.
Apply the same fix as suggested for the playground adapter:
ApiComponent,
{
placeholder: $t('ui.placeholder.select'),
...props,
...attrs,
component: Select,
loadingSlot: 'suffixIcon',
modelPropName: 'value',
- visibleEvent: 'onVisibleChange',
+ visibleEvent: 'onDropdownVisibleChange',
},Line range hint 100-111: Standardize field name configurations across components
The same consideration about standardizing fieldNames configuration applies here. Consider applying consistent field mapping across all select-like components.
apps/web-naive/src/adapter/component/index.ts (1)
Line range hint 73-82: LGTM! Prop mappings for ApiSelect look correct.
The component registration properly maps the NSelect component with correct prop names:
modelPropName: 'value'matches Naive UI's API- Default placeholder is correctly set
packages/effects/common-ui/src/components/api-component/api-component.vue (2)
21-21: LGTM! Type change from VNode to Component is correct.
The type change improves type safety and better represents the component's purpose.
56-56: LGTM! Component name updated correctly.
The component name has been updated to reflect its more generic purpose.
apps/web-ele/src/adapter/component/index.ts (1)
Line range hint 191-193: LGTM: Component registration is properly handled
The component registration to globalShareState is correctly implemented and maintains backward compatibility.
Description
ApiSelect改为ApiComponent，不只是Select组件，ApiComponent也可以包装其它类似组件，为其提供获取远程选项数据的能力。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
ApiComponent, replacing the previousApiSelect, enhancing component registration and usage.ApiComponent, including usage examples and detailed properties.ApiComponentdocumentation.Bug Fixes
ApiSelecttoApiComponent.Chores
ApiSelectcomponent.ApiComponentin various modules.