feat: add scrollToFirstError to the form component #6482
Conversation
Elm1992
requested review from
anncwb,
vince292007,
mynetfan,
jinmao88 and
a team
as code owners
|
WalkthroughA new "scroll to first error" feature was introduced to the form system, allowing forms to automatically scroll to the first error field upon validation failure. This includes API, type, and documentation updates, localization entries, a new demo route and component, and internal logic changes to support and demonstrate the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FormComponent
participant FormApi
User->>FormComponent: Submit form
FormComponent->>FormApi: validate()
alt Validation fails
FormApi->>FormApi: scrollToFirstError(errors)
FormApi->>FormComponent: Return validation errors
FormComponent->>User: Scrolls to first error field
else Validation passes
FormApi->>FormComponent: Return success
FormComponent->>User: Submit success
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (2)
playground/src/views/examples/form/scroll-to-error-test.vue (1)
16-17: Consider reactive scroll-to-error behavior.The
scrollToFirstErrorproperty is set once during form initialization but doesn't react to changes inscrollEnabled. ThetoggleScrollToErrorfunction updates the state, but the initial value won't be reactive.Consider using a computed property for reactive behavior:
-const [Form, formApi] = useVbenForm({ - scrollToFirstError: scrollEnabled.value, +const [Form, formApi] = useVbenForm({ + scrollToFirstError: computed(() => scrollEnabled.value),Or ensure the current approach with
setStateis the intended pattern for this API.packages/@core/ui-kit/form-ui/src/form-api.ts (1)
270-272: Consider more specific DOM element selection.The current selector
[name="${firstErrorFieldName}"]might match unintended elements. Consider adding form context or more specific selectors.- let el = document.querySelector( - `[name="${firstErrorFieldName}"]`, - ) as HTMLElement; + let el = document.querySelector( + `form [name="${firstErrorFieldName}"], [data-field="${firstErrorFieldName}"]`, + ) as HTMLElement;This would provide better specificity and an alternative data attribute approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/src/components/common-ui/vben-form.md(1 hunks)docs/src/demos/vben-form/rules/index.vue(1 hunks)packages/@core/ui-kit/form-ui/src/components/form-actions.vue(1 hunks)packages/@core/ui-kit/form-ui/src/form-api.ts(4 hunks)packages/@core/ui-kit/form-ui/src/types.ts(1 hunks)playground/src/locales/langs/en-US/examples.json(1 hunks)playground/src/locales/langs/zh-CN/examples.json(1 hunks)playground/src/router/routes/modules/examples.ts(1 hunks)playground/src/views/examples/form/scroll-to-error-test.vue(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
playground/src/locales/langs/zh-CN/examples.json (1)
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5587
File: playground/src/views/examples/loading/index.vue:15-18
Timestamp: 2025-02-23T04:21:24.691Z
Learning: Chinese text in the description of the loading component example (`playground/src/views/examples/loading/index.vue`) is intentionally kept without i18n support.
docs/src/demos/vben-form/rules/index.vue (1)
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5397
File: playground/src/bootstrap.ts:23-30
Timestamp: 2025-01-15T04:29:13.944Z
Learning: In the Vue-Vben-Admin project, commented code in bootstrap.ts showing modal and drawer configuration examples should be preserved as reference documentation, even after moving to CSS variables for z-index management.
playground/src/router/routes/modules/examples.ts (1)
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5587
File: playground/src/views/examples/loading/index.vue:15-18
Timestamp: 2025-02-23T04:21:24.691Z
Learning: Chinese text in the description of the loading component example (`playground/src/views/examples/loading/index.vue`) is intentionally kept without i18n support.
playground/src/views/examples/form/scroll-to-error-test.vue (2)
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5397
File: playground/src/bootstrap.ts:23-30
Timestamp: 2025-01-15T04:29:13.944Z
Learning: In the Vue-Vben-Admin project, commented code in bootstrap.ts showing modal and drawer configuration examples should be preserved as reference documentation, even after moving to CSS variables for z-index management.
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5013
File: docs/src/components/layout-ui/page.md:31-31
Timestamp: 2024-12-04T04:41:30.161Z
Learning: 在 Vben Admin 项目的 `Page` 组件中，`extra` 内容仅通过 slots 提供，没有对应的 prop 属性。
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (11)
playground/src/locales/langs/zh-CN/examples.json (1)
25-25: LGTM! Clean localization addition.The Chinese translation for "scrollToError" is accurate and properly placed within the form section.
playground/src/locales/langs/en-US/examples.json (1)
22-22: LGTM! Consistent localization key.The English translation is clear and follows standard UI terminology. Key name matches the Chinese localization file.
docs/src/demos/vben-form/rules/index.vue (1)
18-18: LGTM! Appropriate demo usage.Enabling
scrollToFirstErrorin the rules demo is logical since it showcases validation behavior where the scroll feature would be most visible to users.packages/@core/ui-kit/form-ui/src/types.ts (1)
390-394: LGTM! Well-documented type definition.The property is properly typed as optional boolean with clear JSDoc documentation. The Chinese description accurately explains the functionality.
playground/src/router/routes/modules/examples.ts (1)
88-96: LGTM! Route definition follows established patterns.The new route entry for the scroll-to-error test example is properly structured and consistent with other form example routes.
packages/@core/ui-kit/form-ui/src/components/form-actions.vue (1)
51-63: Excellent refactor for safer form API access.The changes improve code quality by:
- Extracting
propsonce for better performance- Adding a guard clause to prevent errors when
formApiis undefined- Using consistent access patterns throughout the function
This refactor supports the new scroll-to-error functionality while making the code more robust.
playground/src/views/examples/form/scroll-to-error-test.vue (2)
22-23: Verify Chinese text localization approach.The component contains Chinese text in placeholders and descriptions. Please verify this follows the project's localization strategy for example components.
Based on retrieved learnings, Chinese text in examples is sometimes intentionally kept without i18n support. Please confirm if this component should follow the same pattern or use localized strings for consistency.
Also applies to: 31-32, 40-41, 49-50, 58-59, 67-68, 76-77, 89-89
1-183: Excellent demonstration component for scroll-to-error feature.The component provides comprehensive testing capabilities for the new scroll-to-error functionality with:
- Multiple form fields with validation rules
- Interactive toggle for scroll behavior
- Various validation test methods
- Clear documentation and explanations
- Proper Vue 3 composition API usage
packages/@core/ui-kit/form-ui/src/form-api.ts (3)
42-42: Consider default value for scrollToFirstError.The default value is set to
false, but the PR objectives suggest this feature should be enabled by default to address user experience issues. Consider iftruewould be more appropriate.Please confirm the intended default behavior. If users expect automatic scrolling to errors as a standard UX pattern, defaulting to
truemight be more intuitive.
257-290: Well-implemented scroll-to-error functionality with good fallback strategy.The implementation demonstrates solid engineering practices:
- Clear documentation with GitHub issue reference
- Robust element-finding strategy with fallback
- Smooth scrolling with appropriate viewport positioning
- Proper type checking and early returns
The reference to vee-validate discussion (#3835) shows proper research was conducted for this implementation.
417-419: Consistent integration with validation methods.The scroll-to-error functionality is properly integrated across all validation methods (
validate,validateAndSubmitForm,validateField) with consistent conditional logic.Also applies to: 428-430, 443-445
| @@ -324,6 +324,7 @@ useVbenForm 返回的第二个参数，是一个对象，包含了一些表单 | |||
| | submitOnEnter | 按下回车健时提交表单 | `boolean` | false | | |||
| | submitOnChange | 字段值改变时提交表单(内部防抖，这个属性一般用于表格的搜索表单) | `boolean` | false | | |||
| | compact | 是否紧凑模式(忽略为校验信息所预留的空间) | `boolean` | false | | |||
| | scrollToFirstError | 表单验证失败时是否自动滚动到第一个错误字段 | `boolean` | false | | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix default value inconsistency.
There's an inconsistency between the documentation (showing default as false) and the type definition JSDoc comment (showing @default true). Please verify and align the correct default value across both locations.
🏁 Script executed:
#!/bin/bash
# Description: Check the actual default value implementation in form-api.ts and components
# Expected: Find the actual default value used in the implementation
# Search for scrollToFirstError default value in form API implementation
rg -A 5 -B 5 "scrollToFirstError.*=" --type ts
# Search for any mentions of scrollToFirstError in the codebase
rg "scrollToFirstError" --type ts -A 2 -B 2Length of output: 2522
Update JSDoc default to false
The implementation in form-api.ts initializes scrollToFirstError to false, and the docs already reflect a default of false. The JSDoc in the type declaration is out of sync—please update it as follows:
• File: packages/@core/ui-kit/form-ui/src/types.ts
Location: JSDoc above scrollToFirstError
/**
* Whether to automatically scroll to the first error field when validation fails
- * @default true
+ * @default false
*/
scrollToFirstError?: boolean;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | scrollToFirstError | 表单验证失败时是否自动滚动到第一个错误字段 | `boolean` | false | | |
| /** | |
| * Whether to automatically scroll to the first error field when validation fails | |
| * @default false | |
| */ | |
| scrollToFirstError?: boolean; |
🤖 Prompt for AI Agents
In packages/@core/ui-kit/form-ui/src/types.ts at the JSDoc comment above the
scrollToFirstError declaration, update the default value in the JSDoc from true
to false to match the actual initialization in form-api.ts and the documentation
in vben-form.md. This ensures consistency between the code, JSDoc, and docs.
Description
#5396 https://github.com/logaretm/vee-validate/discussions/3835，这个方法正好能解决此问题
这块代码我觉得我直接把form替换成了props.formApi，因为这样可以少写点代码，而且看不出有什么问题。
如果觉得这里有破坏性，那我直接撤回多写一段代码也行，就是觉得有点冗余，而且都是调用验证方法，调用哪里的无所谓吧。
Type of change
Summary by CodeRabbit
New Features
Documentation
Localization