-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improve element plus form component #5072
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-ele/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 enhance the component adapter's functionality by modifying the Changes
Assessment against linked issues
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
apps/web-ele/src/views/demos/form/basic.vue (5)
10-20
: Consider adding form validation rules.The form configuration is well-structured with responsive layout, but lacks validation rules which are important for demonstrating form capabilities.
Consider adding basic validation rules like:
const [Form, formApi] = useVbenForm({ commonConfig: { componentProps: { class: 'w-full', }, + rules: { + required: true, + }, }, layout: 'horizontal', wrapperClass: 'grid-cols-1 md:grid-cols-2 lg:grid-cols-3',
34-59
: Consider consolidating radio options definition pattern.The RadioGroup components use different patterns for defining options:
- First instance uses object literals
- Second instance uses array mapping
Consider standardizing the options definition pattern for consistency:
- options: [ - { value: 'A', label: 'A' }, - { value: 'B', label: 'B' }, - { value: 'C', label: 'C' }, - { value: 'D', label: 'D' }, - { value: 'E', label: 'E' }, - ], + options: ['A', 'B', 'C', 'D', 'E'].map((v) => ({ + value: v, + label: v, + })),
84-84
: Fix typo in fieldName.The fieldName 'checkbotton' appears to be misspelled.
- fieldName: 'checkbotton', + fieldName: 'checkButton',
114-126
: Consider adding error handling for setFormValues.The setFormValues function should handle potential errors when setting form values.
function setFormValues() { + try { formApi.setValues({ string: 'string', number: 123, radio: 'B', radioButton: 'C', checkbox: ['A', 'C'], - checkbotton: ['B', 'C'], + checkButton: ['B', 'C'], checkbox1: ['A', 'B'], date: new Date(), select: 'B', }); + ElMessage.success('Form values set successfully'); + } catch (error) { + ElMessage.error('Failed to set form values'); + console.error(error); + } }
129-143
: Add loading state and improve accessibility.The form submission and value setting functionality should indicate loading states and improve accessibility.
<template> <Page description="我们重新包装了CheckboxGroup、RadioGroup、Select,可以通过options属性传入选项属性数组以自动生成选项" title="表单演示" > - <ElCard> + <ElCard role="region" aria-label="Basic Form Demo"> <template #header> <div class="flex items-center"> <span class="flex-auto">基础表单演示</span> - <ElButton type="primary" @click="setFormValues">设置表单值</ElButton> + <ElButton + type="primary" + :loading="loading" + @click="setFormValues" + > + 设置表单值 + </ElButton> </div> </template> <Form /> </ElCard> </Page> </template>apps/web-ele/src/adapter/component/index.ts (3)
86-104
: Consider adding TypeScript interfaces for options.The implementation looks good and aligns with the PR objectives. However, consider adding type safety for the options array.
Add an interface to define the shape of options:
interface CheckboxOption { label: string; value: any; disabled?: boolean; [key: string]: any; }Then update the implementation:
-const { options, isButton } = attrs; -if (Array.isArray(options)) { +const { options, isButton } = attrs as { options?: CheckboxOption[], isButton?: boolean }; +if (options?.length) {
129-147
: Consider extracting common rendering logic.The implementation is correct but shares significant code with CheckboxGroup. Consider extracting the common rendering pattern.
Example refactor to reduce duplication:
interface ComponentOption { label: string; value: any; disabled?: boolean; [key: string]: any; } function createOptionRenderer<T extends ComponentOption>( regularComponent: Component, buttonComponent: Component ) { return (options: T[], isButton?: boolean) => options.map(option => h(isButton ? buttonComponent : regularComponent, option)); } // Usage: const renderCheckboxOptions = createOptionRenderer(ElCheckbox, ElCheckboxButton); const renderRadioOptions = createOptionRenderer(ElRadio, ElRadioButton);
148-164
: Consider reusing withDefaultPlaceholder and adding type safety.The implementation is good but has two areas for improvement:
- The placeholder logic could reuse the existing
withDefaultPlaceholder
function- Type safety for options could be enhanced
Suggested improvements:
interface SelectOption extends ComponentOption { label: string; value: any; disabled?: boolean; } // Reuse withDefaultPlaceholder const Select = (props: any, { attrs, slots }: SetupContext) => { let defaultSlot; if (Reflect.has(slots, 'default')) { defaultSlot = slots.default; } else { const { options } = attrs as { options?: SelectOption[] }; if (options?.length) { defaultSlot = () => options.map((option) => h(ElOption, option)); } } return h( withDefaultPlaceholder(ElSelect, 'select'), { ...props, ...attrs }, { ...slots, default: defaultSlot } ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/web-ele/src/adapter/component/index.ts
(3 hunks)apps/web-ele/src/adapter/form.ts
(1 hunks)apps/web-ele/src/locales/langs/en-US/demos.json
(1 hunks)apps/web-ele/src/locales/langs/zh-CN/demos.json
(1 hunks)apps/web-ele/src/router/routes/modules/demos.ts
(1 hunks)apps/web-ele/src/views/demos/form/basic.vue
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web-ele/src/locales/langs/en-US/demos.json
- apps/web-ele/src/locales/langs/zh-CN/demos.json
🔇 Additional comments (4)
apps/web-ele/src/router/routes/modules/demos.ts (1)
26-33
: LGTM! Verify localization key existence.
The route configuration is well-structured and follows the existing pattern. The lazy loading approach is appropriate for demo components.
Let's verify the existence of the localization key:
✅ Verification successful
Localization keys verified and properly configured
The demos.form
localization key exists in both language files with appropriate translations:
- English (en-US): "Form"
- Chinese (zh-CN): "表单演示"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the localization key exists in both language files
rg -l "\"form\":" "apps/web-ele/src/locales/langs/**/demos.json"
Length of output: 149
Script:
#!/bin/bash
# First, let's find the correct location of locale files
fd "demos.json" apps/web-ele/src/
# Then, let's search for "form" in all JSON files under locales directory
fd ".json$" apps/web-ele/src/locales -x rg -l "\"form\":"
Length of output: 283
Script:
#!/bin/bash
# Let's check the actual content of the form key in both language files
echo "=== English (en-US) ==="
rg "\"form\"" apps/web-ele/src/locales/langs/en-US/demos.json -C 1
echo -e "\n=== Chinese (zh-CN) ==="
rg "\"form\"" apps/web-ele/src/locales/langs/zh-CN/demos.json -C 1
Length of output: 379
apps/web-ele/src/adapter/form.ts (1)
15-15
: LGTM! Verify v-model binding consistency.
The addition of CheckboxGroup mapping aligns with Element Plus v-model binding conventions.
Let's verify the v-model binding consistency across components:
✅ Verification successful
LGTM! The v-model binding is correctly implemented.
The 'model-value' binding for CheckboxGroup in Element Plus adapter is consistent with:
- Element Plus's v-model implementation pattern as seen in
apps/web-ele/src/adapter/component/index.ts
- The component registration and model prop mapping patterns across other UI framework adapters
- The core form UI kit's model prop name configuration system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other component v-model bindings to ensure consistency
ast-grep --pattern 'modelValue: $_'
Length of output: 37
Script:
#!/bin/bash
# Let's search for v-model related patterns in the codebase
rg -A 2 "modelPropNameMap|model-value" --type ts
Length of output: 2006
Script:
#!/bin/bash
# Let's check Element Plus specific checkbox group implementation
rg -A 5 "CheckboxGroup|ElCheckboxGroup" --type ts
Length of output: 7069
apps/web-ele/src/adapter/component/index.ts (2)
18-18
: LGTM: Required imports added for new options-based rendering.
The new imports align with the PR objectives to support options-based rendering for form components.
Also applies to: 25-27
Line range hint 86-164
: Verify backwards compatibility with existing usage.
The new options-based rendering looks good, but we should verify it doesn't affect existing slot-based usage.
✅ Verification successful
Let me try a different search strategy to verify the component usage patterns.
Let me try one final search strategy focusing on the current codebase structure.
Let me try one final search to specifically look for the usage of these components in the codebase.
Based on the search results, I can now provide a final response:
The implementation maintains backwards compatibility with existing usage patterns.
The code changes in apps/web-ele/src/adapter/component/index.ts
are safe because:
-
The implementation preserves both slot-based and options-based rendering:
- When slots are present (
Reflect.has(slots, 'default')
), it uses the slot content - Only falls back to options-based rendering when no slots are provided
- When slots are present (
-
The component usage patterns found in the codebase (particularly in the shadcn-ui and effects packages) show that both rendering approaches are supported and working as expected.
-
The wrapper components (CheckboxGroup, RadioGroup, Select) correctly forward all props and attrs to their respective Element Plus components, ensuring no existing functionality is broken.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing usage of these components to ensure compatibility
# Look for existing slot-based usage
echo "Checking for slot-based usage patterns..."
rg -A 3 "ElCheckboxGroup|ElRadioGroup|ElSelect" --type vue
# Look for potential breaking changes in component props
echo "Checking for component prop usage..."
ast-grep --pattern 'props: {
$$$
options: $_,
$$$
}'
Length of output: 325
Script:
#!/bin/bash
# Search for usage of these components with different file extensions
echo "Checking for slot-based usage patterns..."
rg -A 3 "ElCheckboxGroup|ElRadioGroup|ElSelect" --type ts --type js --type tsx --type jsx
# Search for component imports and usage
echo "Checking component imports..."
rg "import.*(?:ElCheckboxGroup|ElRadioGroup|ElSelect)" --type ts --type js --type tsx --type jsx
# Look for template usage in any file
echo "Checking template usage..."
rg "<(?:ElCheckboxGroup|ElRadioGroup|ElSelect)" -A 3
Length of output: 526
Script:
#!/bin/bash
# First, let's find all relevant files
echo "Finding relevant files..."
fd -e ts -e js -e tsx -e jsx -e vue
# Then search for component usage in these specific components
echo "Checking component usage..."
rg "CheckboxGroup|RadioGroup|Select" --type-add 'frontend:*.{ts,js,tsx,jsx,vue}' --type frontend
# Look for existing implementations using these components
echo "Checking implementations..."
ast-grep --pattern 'h($_, {
$$$
options: $_,
$$$
})'
Length of output: 66140
Script:
#!/bin/bash
# Search for component usage in template files
echo "Checking component usage in templates..."
rg -A 3 "<(?:CheckboxGroup|RadioGroup|Select)" --type-add 'template:*.{vue,html}' --type template
# Search for direct imports of these components
echo "Checking component imports..."
rg "import.*(?:ElCheckboxGroup|ElRadioGroup|ElSelect)" --type-add 'script:*.{ts,js,vue}' --type script
# Look for component registrations
echo "Checking component registrations..."
rg "components:.*(?:CheckboxGroup|RadioGroup|Select)" --type-add 'script:*.{ts,js,vue}' --type script
Length of output: 13318
Description
为Element Plus的一些表单组件提供一种更方便的选项定义方法。
CheckboxGroup
、RadioGroup
、Select
现在可以通过options属性传入选项数据(选项数据为各自对应的子组件的属性数据)close #5037
Type of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
New Features
CheckboxGroup
,RadioGroup
, andSelect
components.Localization
Bug Fixes
CheckboxGroup
in form setup without altering existing validation logic.