fix(form): modify mobile problem #2665
Conversation
WalkthroughThe pull request introduces modifications to the mobile form components across multiple files. The changes primarily focus on updating the form validation logic in the Changes
Sequence DiagramsequenceDiagram
participant Form as Mobile Form
participant Validation as Validation Rules
participant FormItem as Form Item
Form->>Validation: Define rules
FormItem->>Validation: Bind prop
FormItem->>Form: Trigger validation
Validation->>FormItem: Validate input
Validation-->>FormItem: Show error message if invalid
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🔭 Outside diff range comments (1)
examples/sites/demos/mobile/app/form/validate-position.vue (1)
Line range hint
1-24: Add unit tests for form validationAs mentioned in the PR objectives, tests are missing. Please add unit tests to verify:
- Form validation behavior
- Error message display position
- Mobile-specific functionality
Would you like me to help generate the test cases for these scenarios?
🧹 Nitpick comments (2)
packages/mobile/components/form/src/mobile.vue (1)
35-36: Consider consistent naming approach between name and componentNameThere's an inconsistency between
name(using$prefix) andcomponentName(hardcoded). This might cause issues with component identification or tree-shaking.Consider using one of these approaches:
name: $prefix + 'Form', - componentName: 'Form', + componentName: $prefix + 'Form',or
name: $prefix + 'Form', - componentName: 'Form', + componentName: name,examples/sites/demos/mobile/app/form/validate-position.vue (1)
Line range hint
12-18: Consider improving field labels for better UXBoth form items use the same label "优秀" which may confuse users. Consider using distinct, descriptive labels for each field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/sites/demos/mobile/app/form/validate-position.vue(2 hunks)packages/mobile/components/form/src/mobile.vue(1 hunks)packages/mobile/components/form/src/renderless/index.ts(1 hunks)packages/mobile/components/form/src/renderless/vue.ts(1 hunks)
🔇 Additional comments (4)
packages/mobile/components/form/src/mobile.vue (2)
28-28: LGTM! Verify vue-common package usage
The import change appropriately brings in the required utilities for component naming standardization.
✅ Verification successful
Let me verify the specific imports from vue-common to ensure consistency.
Import changes are consistent with mobile component patterns
The import change in the form component aligns perfectly with the established patterns across the mobile component suite. The results show that:
- All mobile components consistently import from '../../../vue-common'
- The combination of
$prefix,setup, anddefineComponentis a common pattern used in multiple components (action-sheet, list, mini-picker, etc.) - Some components use additional utilities like
props,h, ordirectiveas needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of vue-common imports across mobile components
rg -g '*.vue' -l "import.*from '\.\./\.\./\.\./vue-common'" packages/mobile/components/
Length of output: 2708
Script:
#!/bin/bash
# Check the specific imports ($prefix, setup, defineComponent) in mobile components
rg -g '*.vue' "import.*\{.*(\$prefix|setup|defineComponent).*\}.*from '\.\./\.\./\.\./vue-common'" packages/mobile/components/
Length of output: 6274
Line range hint 1-43: Please clarify the bug fix and add tests
While the code changes look technically sound, there are some concerns:
- The relationship between these naming changes and the reported mobile form bug isn't clear
- The PR is marked as a bugfix, but the changes appear more like refactoring
- No tests have been added to verify the fix
Could you please:
- Explain how these changes fix the mobile problem
- Add tests to verify the fix
packages/mobile/components/form/src/renderless/vue.ts (1)
96-96: LGTM: Parameter change consistent with renderless implementation
The update to use vm instead of parent maintains consistency with the changes in index.ts.
examples/sites/demos/mobile/app/form/validate-position.vue (1)
Line range hint 8-24: Verify mobile-specific form behavior
Since this PR fixes a mobile form issue:
- Ensure the form is properly responsive
- Verify touch event handling
- Test validation position on mobile viewports
✅ Verification successful
Mobile form validation position is properly implemented
The code inspection reveals that:
- The validation position feature is correctly implemented in the mobile form components
- The component properly handles both left and right validation message positioning
- The validation position is inherited from the form instance with a fallback to 'right'
- The CSS classes are properly set for both left and right alignment
The implementation aligns with the changes in the demo file, and the validation position feature appears to be working as intended for mobile viewports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mobile-specific form tests and implementations
rg -l "mobile.*form|form.*mobile" --type vue --type test
# Look for viewport-related handling
rg -l "validatePosition|validate-position" --type vue --type ts
Length of output: 180
Script:
#!/bin/bash
# Search for mobile-specific form tests and implementations with proper file extensions
rg -l "mobile.*form|form.*mobile" -g "*.vue" -g "*.test.*" -g "*.spec.*"
# Look for viewport-related handling and validation position
rg -l "validatePosition|validate-position" -g "*.vue" -g "*.ts" -g "*.js"
# Search for form validation and mobile-related code patterns using ast-grep
ast-grep --pattern 'validate-position="$_"'
# Look for mobile-specific form components and tests
fd -e vue -e js -e ts . | xargs rg -l "tiny-form.*mobile|mobile.*tiny-form"
Length of output: 2398
Script:
#!/bin/bash
# Check implementation details of mobile form validation
rg -A 10 "validate-position" packages/mobile/components/form/src/form.ts
rg -A 10 "validate-position" packages/mobile/components/form-item/src/form-item.ts
# Look for mobile-specific validation handling
rg -A 5 "validatePosition" packages/vue/src/form/src/mobile.vue
rg -A 5 "validatePosition" packages/mobile/components/form-item/src/mobile.vue
# Check for any mobile viewport or touch-specific handling
rg "mobile|touch" packages/mobile/components/form/src/form.ts
Length of output: 911
| ({ vm, state }: Pick<IFormRenderlessParams, 'vm' | 'state'>) => | ||
| (): void => { | ||
| parent.$on('form:addField', (field) => { | ||
| vm.$on('form:addField', (field) => { | ||
| if (field) { | ||
| state.fields.push(field) | ||
| } | ||
| }) | ||
| /* istanbul ignore next */ | ||
| parent.$on('form:removeField', (field) => { | ||
| vm.$on('form:removeField', (field) => { | ||
| if (field.prop) { | ||
| state.fields.splice(state.fields.indexOf(field), 1) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Parameter rename from parent to vm requires coordinated updates
The search results reveal an inconsistency that needs to be addressed. While packages/mobile/components/form/src/renderless/index.ts has been updated to use vm, the base implementation in packages/renderless/src/form/index.ts still uses parent. This mismatch could lead to compatibility issues:
packages/renderless/src/form/index.ts: Usesparent.$on('form:addField')packages/mobile/components/form/src/renderless/index.ts: Usesvm.$on('form:addField')- Both
packages/renderless/src/form-item/index.tsandpackages/mobile/components/form-item/src/renderless/index.tsdispatch these events viaapi.dispatch('Form', 'form:addField', vm)
The parameter rename should be applied consistently across all related files to maintain compatibility.
🔗 Analysis chain
Verify event handling compatibility after parameter rename
The change from parent to vm in the created method affects form field registration events. While this change aligns better with Vue's component instance naming conventions, we should ensure backward compatibility.
Consider adding a deprecation warning if parent is passed instead of vm to ensure smooth migration for existing implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom implementations that might still use the old parent parameter
rg -l "form:addField|form:removeField" --type vue
Length of output: 79
Script:
#!/bin/bash
# Search for form event usage across all files
rg -l "form:addField|form:removeField"
Length of output: 238
Script:
#!/bin/bash
# Search for actual usage of these events to understand the context
rg "form:addField|form:removeField" -A 2 -B 2
Length of output: 3202
Script:
#!/bin/bash
# Look for any imports or references to the form component
rg "from.*form.*renderless"
Length of output: 29
Script:
#!/bin/bash
# Check the history of this specific file to understand the change
git log -p -- packages/mobile/components/form/src/renderless/index.ts | head -n 50
Length of output: 0
| users: [{ required: true, message: '必填', trigger: 'change' }], | ||
| user: [{ required: true, message: '必填', trigger: 'change' }] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Internationalize validation messages
The validation messages are hardcoded in Chinese. Consider using i18n for better internationalization support.
- users: [{ required: true, message: '必填', trigger: 'change' }],
- user: [{ required: true, message: '必填', trigger: 'change' }]
+ users: [{ required: true, message: this.$t('form.required'), trigger: 'change' }],
+ user: [{ required: true, message: this.$t('form.required'), trigger: 'change' }]Committable suggestion skipped: line range outside the PR's diff.
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
userinput field.watchRulesmethod to the exported API for better reactive behavior.Bug Fixes
Chores