fix: form fieldMappingTime improve and modelPropName support
#5335
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
playground/src/views/examples/form/custom.vueOops! Something went wrong! :( ESLint: 9.17.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThis pull request introduces enhancements to the Vben Form component's functionality and documentation. The changes focus on improving the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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
🧹 Nitpick comments (6)
playground/src/views/examples/form/modules/two-fields.vue (6)
1-2: Consider importing component types for better type safety.Import the types from ant-design-vue to ensure proper type checking:
<script lang="ts" setup> -import { Input, Select } from 'ant-design-vue'; +import type { InputProps, SelectProps } from 'ant-design-vue'; +import { Input, Select } from 'ant-design-vue';
6-8: Enhance type safety with string literal types for country codes.Consider using more specific types for better type safety and validation:
-const modelValue = defineModel<[string, string]>({ +type CountryCode = '+82' | '+85' | '+86'; +const modelValue = defineModel<[CountryCode, string]>({ default: ['+85', ''], });
10-12: Add validation before emitting the change event.Consider validating the phone number format before emitting the change event:
function onChange() { + const [countryCode, phone] = modelValue.value; + const isValid = phone.match(/^1[3-9]\d{9}$/); + if (!isValid) { + console.warn('Invalid phone number format'); + } emit('change', modelValue.value); }
30-31: Consider implementing i18n for placeholder text.The placeholder text is hardcoded in Chinese. Consider using i18n for better internationalization support.
- placeholder="请输入11位手机号码" + :placeholder="t('form.phoneNumber.placeholder')"
32-38: Improve phone input validation and accessibility.Several improvements can be made to enhance validation and accessibility:
- Extract the regex pattern to a constant
- Add
patternattribute for better mobile validation- Add ARIA attributes for validation status
+const PHONE_PATTERN = /^1[3-9]\d{9}$/; +const isValidPhone = computed(() => modelValue.value[1]?.match(PHONE_PATTERN)); <Input placeholder="请输入11位手机号码" class="flex-1" - :class="{ 'valid-success': modelValue[1]?.match(/^1[3-9]\d{9}$/) }" + :class="{ 'valid-success': isValidPhone }" v-model:value="modelValue[1]" :maxlength="11" type="tel" + :pattern="PHONE_PATTERN.source" + :aria-invalid="!isValidPhone" + :aria-describedby="!isValidPhone ? 'phone-error' : undefined" @blur="emit('blur')" @change="onChange" />
1-40: Consider making the component more reusable.To improve reusability, consider:
- Accepting props for validation patterns
- Allowing customization of country codes
- Supporting different phone number formats
This would make the component more flexible for different use cases and regions. Would you like me to provide an example implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/src/components/common-ui/vben-form.md(2 hunks)packages/@core/ui-kit/form-ui/src/form-api.ts(1 hunks)packages/@core/ui-kit/form-ui/src/form-render/form-field.vue(2 hunks)packages/@core/ui-kit/form-ui/src/form-render/form.vue(2 hunks)packages/@core/ui-kit/form-ui/src/types.ts(4 hunks)packages/styles/src/antd/index.css(2 hunks)playground/src/views/examples/form/custom.vue(4 hunks)playground/src/views/examples/form/modules/two-fields.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
🔇 Additional comments (12)
playground/src/views/examples/form/custom.vue (3)
21-21: LGTM! Good use of fieldMappingTime with null format.The configuration correctly demonstrates the new feature allowing original value mapping without formatting.
45-48: LGTM! Proper implementation of modelPropName.The
modelPropName: 'value'configuration correctly demonstrates the new two-way binding property feature.
57-74: Well-structured validation rules with clear error messages.The Zod validation rules are well-implemented with:
- Proper array length validation
- Clear error messages
- Smart use of full-width space for error message positioning
packages/@core/ui-kit/form-ui/src/types.ts (2)
200-204: Well-documented modelPropName property.The type definition includes proper JSDoc documentation with default value information.
227-227: LGTM! Proper type support for null format in FieldMappingTime.The type correctly uses
Nullable<string>to support the new null format feature.packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (1)
206-208: LGTM! Proper fallback logic for modelPropName.The implementation correctly prioritizes:
- Custom modelPropName
- Component-specific binding from map
packages/@core/ui-kit/form-ui/src/form-api.ts (1)
371-385: LGTM! Clean implementation of null format support.The implementation:
- Properly handles null format case
- Maintains existing functionality for formatted cases
- Correctly supports array format for different start/end formats
packages/styles/src/antd/index.css (2)
24-24: Improved validation style specificity for select componentsThe selector now correctly excludes valid states by checking both the parent and selector elements for the
.valid-successclass.
43-45: Consistent validation styling for input componentsThe new rule for input validation follows the same pattern as select components, ensuring consistent visual feedback across form elements.
packages/@core/ui-kit/form-ui/src/form-render/form.vue (1)
101-101: LGTM: Well-implemented model property name supportThe addition of
modelPropNamewith proper defaults and merging behavior enables flexible two-way binding support for custom components.Also applies to: 122-122
docs/src/components/common-ui/vben-form.md (2)
319-319: Well-documented fieldMappingTime enhancementThe documentation clearly explains the broadened scope and null format behavior, with a helpful example that illustrates the usage pattern.
Also applies to: 325-329
415-419: Clear modelPropName documentationThe documentation effectively explains the purpose and default behavior of the modelPropName property.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
playground/src/views/examples/form/modules/two-fields.vue (3)
4-8: Consider adding runtime type validation for the model value.While the TypeScript type is correct, consider adding runtime validation to ensure the tuple values are either undefined or strings.
+import type { PropType } from 'vue'; + const modelValue = defineModel<[string, string]>({ default: () => [undefined, undefined], + validator: (value): value is [string | undefined, string | undefined] => { + return Array.isArray(value) && + value.length === 2 && + value.every(v => v === undefined || typeof v === 'string'); + } });
34-34: Improve phone number validation pattern.The current regex pattern
^1[3-9]\d{9}$is specific to Chinese phone numbers. Consider making it more flexible or configurable for international phone numbers.-:class="{ 'valid-success': modelValue[1]?.match(/^1[3-9]\d{9}$/) }" +const phonePattern = computed(() => props.phonePattern || /^1[3-9]\d{9}$/); +:class="{ 'valid-success': modelValue[1]?.match(phonePattern.value) }"
21-21: Consider using scoped CSS classes.The
valid-successclass appears to be a global CSS class. Consider using scoped CSS or CSS modules to avoid potential naming conflicts.+<style scoped> +.field-valid { + border-color: #52c41a; +} +</style> -:class="{ 'valid-success': !!modelValue[0] }" +:class="{ 'field-valid': !!modelValue[0] }" -:class="{ 'valid-success': modelValue[1]?.match(/^1[3-9]\d{9}$/) }" +:class="{ 'field-valid': modelValue[1]?.match(/^1[3-9]\d{9}$/) }"Also applies to: 34-34
playground/src/views/examples/form/custom.vue (1)
57-77: Consider improving error message alignment.While the validation rules are well-structured, using full-width spaces ( ) for visual alignment is a fragile solution that may break with different fonts or screen sizes.
Consider using CSS for proper error message alignment:
- message: ' 输入手机号码', + message: '输入手机号码',And add CSS classes to style the error messages:
.ant-form-item-explain-error { margin-left: 8em; /* Adjust based on your layout */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playground/src/views/examples/form/custom.vue(4 hunks)playground/src/views/examples/form/modules/two-fields.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
🔇 Additional comments (6)
playground/src/views/examples/form/modules/two-fields.vue (2)
1-2: LGTM! Clean imports and proper TypeScript setup.
10-12: LGTM! Clean event handling implementation.playground/src/views/examples/form/custom.vue (4)
2-10: LGTM! Well-structured imports with performance optimization.The addition of
markRawshows attention to performance, and the integration of Zod (z) for validation is a good choice for type-safe form validation.
45-50: LGTM! Proper implementation of modelPropName.The addition of
modelPropName: 'value'aligns with the PR objective of supporting custom two-way binding property names.
Line range hint
91-101: LGTM! Clean template implementation.The template section properly uses slots and follows Vue best practices for component composition.
21-21: Verify the field mapping implementation.The
fieldMappingTimemapping withnullformatter aligns with the PR objective of supporting non-datetime components. However, ensure that the mapped fieldsphoneTypeandphoneNumberare properly handled in the parent component.Run this script to verify the field handling:
✅ Verification successful
Example implementation verified
The
fieldMappingTimeconfiguration incustom.vueis a valid example demonstrating how to map multiple fields with a null formatter. Since this is in the examples directory and serves as a demonstration, the implementation is appropriate for its purpose.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the parent component properly handles these fields ast-grep --pattern 'phoneType|phoneNumber'Length of output: 44
Script:
#!/bin/bash # Search for phoneType and phoneNumber fields in Vue files echo "=== Searching for field usage in Vue files ===" rg -U "phoneType|phoneNumber" --type vue # Search for specific field mapping patterns echo -e "\n=== Searching for field mapping configurations ===" ast-grep --pattern 'fieldMapping = $_' ast-grep --pattern 'fieldMappingTime = $_' # Check for related component definitions echo -e "\n=== Searching for related component definitions ===" rg -U "field4.*component" --type vueLength of output: 562
Script:
#!/bin/bash # Search in the specific directory and its parent echo "=== Searching for field usage in the form directory ===" rg -U "phoneType|phoneNumber" playground/src/views/examples/form/ # Search for field4 related configurations echo -e "\n=== Searching for field4 configurations ===" rg -U "field4" playground/src/views/examples/form/ # List all files in the form directory for context echo -e "\n=== Files in the form directory ===" ls -la playground/src/views/examples/form/Length of output: 1855
Description
fieldMappingTime支持将格式化掩码设为null以便原值映射，这样可以支持非日期时间类型的组件；modelPropName设置组件的双向绑定属性名，用于支持未提前注册的双向绑定属性为非默认名称的组件。close #5297
close #5290
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
Documentation
fieldMappingTimepropertyNew Features
modelPropNameproperty for more flexible form field configurationTwoFieldscomponent for enhanced form input handlingImprovements
Style Changes