fix: fix known issues with the form #4696
Conversation
|
WalkthroughThe changes in this pull request primarily focus on enhancing the validation rules and rendering logic within various Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (16)
packages/@core/ui-kit/shadcn-ui/src/components/expandable-arrow/expandable-arrow.vue (1)
Line range hint
20-20: Consider removing commented-out codeThere's a commented-out span element in the template. If this code is no longer needed, it would be best to remove it for code cleanliness.
Consider removing this line:
- <!-- <span>{{ isExpanded ? '收起' : '展开' }}</span> -->packages/@core/ui-kit/form-ui/src/form-render/expandable.ts (1)
Line range hint
1-108: Consider adding comments and improving type safetyThe changes made improve the functionality of the
useExpandablefunction. However, to enhance maintainability and type safety, consider the following suggestions:
- Add comments explaining the purpose of the
isCalculatedref and how it's used throughout the function.- Consider using more specific types for the
rowMappingref instead ofRecord<number, number>. For example, you could define a custom type that better represents the structure of the row mapping.- The
calculateRowMappingfunction is quite complex. Consider breaking it down into smaller, more focused functions to improve readability and maintainability.Would you like assistance in implementing any of these suggestions?
apps/web-naive/src/views/_core/authentication/register.vue (1)
70-73: Improved styling consistency and code readability.The changes to the policy agreement checkbox rendering are beneficial:
- The use of the 'vben-link' class suggests a move towards a more standardized styling approach, improving consistency across the application.
- The simplification of the privacy policy and terms text rendering improves code readability.
These changes align with best practices for maintainability and consistency in UI components.
Consider separating the privacy policy and terms text into two distinct translated strings for better flexibility in internationalization. This would allow for different word orders in various languages. For example:
`${$t('authentication.privacyPolicy')} ${$t('authentication.and')} ${$t('authentication.terms')}`This approach would require adding a new translation key for 'and', but it would provide more flexibility for translations in different languages.
apps/web-antd/src/views/_core/authentication/register.vue (1)
70-70: Consistent styling update for linkThe update to use the
vben-linkclass aligns with the new design system and improves consistency across components. This change simplifies the code and centralizes styling definitions.Consider adding a comment explaining the
vben-linkclass's purpose for better maintainability:class="vben-link ml-1" <!-- vben-link applies cursor-pointer, text-primary, and hover styles -->apps/web-ele/src/views/_core/authentication/register.vue (3)
Line range hint
49-53: Improved validation for confirmPassword field.The updated validation rule enhances error handling by providing a specific required error message and a minimum length check. This change improves the user experience by offering more precise feedback.
Consider extracting the error message to a constant or a separate localization file for better maintainability:
const PASSWORD_TIP = $t('authentication.passwordTip'); // Then use it in the validation rule z.string({ required_error: PASSWORD_TIP }) .min(1, { message: PASSWORD_TIP })
70-73: Improved styling and readability for policy agreement link.The updated class name 'vben-link ml-1' aligns the styling with what appears to be a new design system. The simplified rendering of the link content improves code readability.
Consider adding an aria-label to the link for better accessibility:
h( 'a', { class: 'vben-link ml-1', href: '', 'aria-label': $t('authentication.policyLinkAriaLabel'), }, `${$t('authentication.privacyPolicy')} & ${$t('authentication.terms')}`, )Also, ensure that the 'href' attribute is populated with the correct URL for the privacy policy and terms page.
Missing Unit Tests for Register Component
The
register.vuecomponent inapps/web-ele/src/views/_core/authentication/register.vuedoes not have corresponding unit tests to cover the new validation logic. Adding unit tests will ensure that the validation changes work as expected and help prevent future regressions.
- Implement unit tests for
register.vueto cover the updated validation rules and form behaviors.🔗 Analysis chain
Line range hint
1-95: Overall improvements to form validation and styling.The changes in this file successfully address the PR objectives by enhancing form validation and improving styling consistency. The updates to the confirmPassword validation and the policy agreement link rendering contribute to a better user experience.
To ensure these changes are effective and don't introduce any regressions:
Verify that the new validation messages are displayed correctly:
Check for consistent usage of the 'vben-link' class across the project:
Ensure that unit tests for this component are updated to cover the new validation logic:
Please review the results of these checks and update tests if necessary.
Please run the following corrected scripts to gather the necessary information:
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of authentication.passwordTip translation key rg "authentication.passwordTip" --type vueLength of output: 70
Script:
#!/bin/bash # Search for usage of 'vben-link' class rg "class.*vben-link" --type vueLength of output: 62
Script:
#!/bin/bash # Search for test files related to register component fd "register.spec" --type fileLength of output: 30
Script:
#!/bin/bash # Search for usage of authentication.passwordTip translation key in .vue files rg "authentication.passwordTip" -g "*.vue"Length of output: 2288
Script:
#!/bin/bash # Search for usage of 'vben-link' class in .vue files rg "class.*vben-link" -g "*.vue"Length of output: 1150
Script:
#!/bin/bash # Search for test files related to the register component with common test file extensions fd "register.spec" -e js -e ts -e jsx -e tsxLength of output: 44
playground/src/views/_core/authentication/register.vue (2)
49-49: Improved validation rule for confirmPasswordThe addition of a specific required error message enhances the user experience by providing clearer feedback. This change is consistent with best practices in form validation.
Consider adding a comment explaining the purpose of this specific validation rule to improve code maintainability.
70-70: Simplified link stylingThe use of the
vben-linkclass aligns with the new styling approach and improves consistency across components.Remove the trailing space in the class string:
- class: 'vben-link ml-1 ', + class: 'vben-link ml-1',packages/@core/base/design/src/css/global.css (1)
145-147: Approve the addition of.vben-linkclass with a minor suggestion.The new
.vben-linkclass is a great addition that provides consistent styling for links across the project. It aligns well with the design system approach by using color variables and ensures visual interactivity.To further improve accessibility, consider adding a
focusstate to the class:.vben-link { - @apply text-primary hover:text-primary-hover active:text-primary-active cursor-pointer; + @apply text-primary hover:text-primary-hover active:text-primary-active focus:outline-none focus:ring-2 focus:ring-primary-focus cursor-pointer; }This addition will enhance keyboard navigation and meet WCAG 2.1 success criterion 2.4.7 (Focus Visible).
packages/effects/common-ui/src/ui/authentication/login.vue (2)
172-172: Approve consistent styling approach.The class attribute change for the "Create Account" link mirrors the earlier modification, demonstrating a consistent approach to link styling throughout the component. This change enhances the overall maintainability of the codebase.
Consider documenting the
vben-linkclass in your style guide or component library to ensure consistent usage across the project.
Line range hint
1-180: Summary of changes and suggestion for broader refactoring.The changes in this file focus on simplifying and standardizing the styling of links by introducing the
vben-linkclass. This approach improves code readability and maintainability without altering the component's functionality.Consider applying this styling approach consistently across other components in the project. This could be achieved through a systematic refactoring process:
- Identify all instances of link styling similar to the old pattern.
- Replace them with the new
vben-linkclass.- Update any related documentation or style guides.
Would you like assistance in creating a script to identify potential refactoring targets across the project?
packages/effects/common-ui/src/ui/about/about.vue (1)
Line range hint
1-174: Summary: Styling updates improve consistencyThe changes in this file are part of a broader styling update across the project. The introduction of the
'vben-link'class for links improves consistency in the UI. These changes are minimal and do not affect the logic or functionality of the component.Consider documenting the new
'vben-link'class in your project's style guide or component library documentation to ensure consistent usage across the project.playground/src/router/routes/modules/examples.ts (1)
204-230: LGTM! Consider minor adjustment for consistency.The new routes for ModalExample, DrawerExample, and EllipsisExample are well-structured and consistent with the existing routes in the file. They follow good practices such as:
- Using dynamic imports for components
- Providing localized titles with the $t function
- Including appropriate icons in the meta information
For complete consistency with other multi-route sections (like FormExample and VxeTableExample), consider wrapping these new routes in a parent route object with its own meta information. This would allow for better organization if more related routes are added in the future. Here's an example structure:
{ name: 'MiscExamples', path: '/examples/misc', meta: { icon: 'some-icon', title: $t('examples.misc.title'), }, children: [ // ModalExample, DrawerExample, and EllipsisExample routes here ], },This is a minor suggestion and not necessary for the current implementation to work correctly.
packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (2)
196-196: LGTM: Updated formFieldProps typeThe change from
Record<string, any>toRecordable<any>for formFieldProps is consistent with the new import and likely part of a type standardization effort.Consider using a more specific type than
anyif possible, to improve type safety. For example, if you know the shape of the props, you could use an interface or type alias instead.
Line range hint
200-221: LGTM: Updated fieldBindEvent function signature and inner typesThe change from
Record<string, any>toRecordable<any>for the slotProps parameter is consistent with previous changes. The same update for theeparameter in the onChange handler is also appropriate.Consider using more specific types for slotProps and the onChange event object if their structures are known. This could improve type safety and developer experience.
Also applies to: 227-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- apps/web-antd/src/views/_core/authentication/register.vue (2 hunks)
- apps/web-ele/src/views/_core/authentication/register.vue (2 hunks)
- apps/web-naive/src/views/_core/authentication/register.vue (2 hunks)
- packages/@core/base/design/src/css/global.css (1 hunks)
- packages/@core/ui-kit/form-ui/src/form-render/expandable.ts (2 hunks)
- packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (5 hunks)
- packages/@core/ui-kit/form-ui/src/form-render/form-label.vue (1 hunks)
- packages/@core/ui-kit/shadcn-ui/src/components/expandable-arrow/expandable-arrow.vue (1 hunks)
- packages/effects/common-ui/src/ui/about/about.vue (2 hunks)
- packages/effects/common-ui/src/ui/authentication/login.vue (2 hunks)
- packages/effects/common-ui/src/ui/authentication/register.vue (1 hunks)
- playground/src/router/routes/modules/examples.ts (1 hunks)
- playground/src/views/_core/authentication/register.vue (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/effects/common-ui/src/ui/authentication/register.vue
🧰 Additional context used
🔇 Additional comments (19)
packages/@core/ui-kit/form-ui/src/form-render/form-label.vue (1)
15-15: Confirm the intention behind removing the bottom margin.The 'mb-1' class has been removed from the FormLabel component, which eliminates the bottom margin of the label. While this could be part of the styling adjustments mentioned in the PR summary, it's important to ensure this change doesn't negatively impact the spacing between labels and form fields.
Could you please clarify the reasoning behind this change? Is it part of a broader update to the spacing system?
It would be beneficial to verify the visual impact of this change across different form layouts to ensure consistent and appropriate spacing. Consider running the following script to find all usages of this component:
This will help identify all instances where this component is used, allowing for a comprehensive review of the spacing impact.
packages/@core/ui-kit/shadcn-ui/src/components/expandable-arrow/expandable-arrow.vue (3)
Line range hint
1-33: Component logic looks goodThe overall component logic is well-implemented:
- The use of
defineModelfor thecollapsedstate is a modern and efficient approach in Vue 3.- The click handler effectively toggles the
collapsedstate.- The slot usage provides flexibility for the parent component to control the content based on the expanded state.
- The transition effect for the arrow rotation is smoothly implemented.
Good job on creating a reusable and flexible component!
Line range hint
1-33: Overall assessment: Good improvements with minor suggestionsThe changes made to this component, particularly the adoption of the 'vben-link' class, align well with the project's updated styling approach. The component logic is well-implemented, providing good flexibility through slot usage and efficient state management.
A few minor suggestions:
- Ensure that the 'vben-link' class provides all necessary styles previously included in the removed classes.
- Consider removing the commented-out code for improved cleanliness.
Great job on maintaining and improving this component!
15-15: Class binding updated to use 'vben-link'The class binding has been simplified and now uses the 'vben-link' class. This change aligns with the project's recent updates to link classes across various components, as mentioned in the AI summary.
However, we should verify if this change maintains all the necessary styles for the component.
Let's check if the 'vben-link' class is defined and includes the necessary styles:
packages/@core/ui-kit/form-ui/src/form-render/expandable.ts (2)
25-25: Approve: Ensure at least one form item is always visibleThis change improves the robustness of the
keepFormItemIndexcomputed property. By returningmaxItem - 1 || 1, it guarantees that at least one form item will always be visible, even in edge cases where the calculation might result in zero items. This prevents potential issues with an empty form display.
39-39: Approve: Reset calculation state on relevant changesThis addition ensures that the
isCalculatedstate is reset tofalsewhenever the collapse button visibility, active breakpoint, or schema length changes. This is a good practice as it forces a fresh recalculation of the row mapping when any of these critical properties change, ensuring that the form layout always reflects the current state accurately.apps/web-naive/src/views/_core/authentication/register.vue (1)
49-49: Improved error handling for empty confirm password field.The updated validation rule now explicitly sets a required error message using
z.string({ required_error: ... }). This change enhances the user experience by providing a specific error message when the confirm password field is left empty, while still maintaining the minimum length check.apps/web-antd/src/views/_core/authentication/register.vue (3)
49-49: Improved error handling for confirmPassword fieldThe addition of the
required_errorproperty enhances the validation by providing a specific error message when the field is empty. This improvement aligns with best practices in form validation and contributes to a better user experience.
73-73: Improved readability for privacy policy and terms linkThe use of a template literal to concatenate the privacy policy and terms text improves code readability and maintainability. This change aligns with modern JavaScript best practices and reduces the potential for errors in string concatenation.
Line range hint
49-73: Overall improvements to form validation and stylingThe changes in this file effectively address the PR objectives by enhancing form validation, maintaining styling consistency, and improving code readability. The modifications to the confirmPassword field validation, link styling, and text rendering contribute to a better user experience and more maintainable code. These updates align well with modern best practices and the project's design system.
playground/src/views/_core/authentication/register.vue (2)
73-73: Improved content rendering for checkboxThe streamlined approach using a template string enhances code readability while maintaining proper localization. This change simplifies the component and makes it easier to maintain.
Line range hint
1-95: Overall assessment: Improvements to form validation and renderingThe changes in this file enhance the registration form by:
- Improving validation rules with more specific error messages.
- Aligning styling with the new
vben-linkclass for consistency.- Simplifying content rendering for better readability and maintainability.
These modifications contribute to a better user experience and more maintainable code. The changes are well-aligned with the project's localization strategy and styling approach.
packages/effects/common-ui/src/ui/authentication/login.vue (1)
123-123: Approve styling change and suggest verification.The simplification of the class attribute for the "Forget Password" link improves code readability and maintainability. The new
vben-linkclass likely encapsulates the previously explicit styling, including hover and active states.To ensure consistency, please verify the implementation of the
vben-linkclass:packages/effects/common-ui/src/ui/about/about.vue (2)
117-117: LGTM! Consistent styling applied.The update of the GitHub link's class to
'vben-link'is consistent with the earlier change in therenderLinkfunction. This ensures uniform styling for all links in the component.
47-47: LGTM! Verify the new link styling.The change from
'text-primary hover:text-primary-hover'to'vben-link'aligns with the project-wide styling updates. This should lead to more consistent link styling across the application.To ensure the new styling is applied correctly, please run the following script to check the definition of the
vben-linkclass:✅ Verification successful
Verified: New link styling is correctly defined.
The
vben-linkclass is properly defined inpackages/@core/base/design/src/css/global.css, ensuring consistent styling across the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of the 'vben-link' class # Test: Search for the 'vben-link' class definition rg -g '*.{css,scss,less,styl}' '\.vben-link'Length of output: 107
packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (4)
2-2: LGTM: Import of Recordable typeThe addition of the
Recordabletype import from '@vben-core/typings' is a good practice. It enhances type safety and clarity in the rest of the file.
235-235: LGTM: Updated createComponentProps function signatureThe change from
Record<string, any>toRecordable<any>for the slotProps parameter is consistent with previous changes and maintains type consistency throughout the file.
283-283: LGTM: Updated FormLabel class bindingThe addition of 'mb-1' class when isVertical is true is a good styling adjustment. It adds a margin-bottom to the FormLabel in vertical layout, which should improve the visual spacing.
Line range hint
1-353: Overall assessment: Approved with minor suggestionsThe changes in this file consistently improve type safety by using the
Recordabletype and make a minor styling adjustment. The modifications are well-aligned with each other and likely part of a larger effort to standardize types across the project.While the changes are approved, consider the suggestions for using more specific types where possible to further enhance type safety and developer experience.
* fix: fix known issues with the form * chore: typo * chore: typo
Description
fixed #4498
fixed #4681
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
confirmPasswordfield with clearer error messaging.Bug Fixes
Style
vben-linkclass.Documentation
Chores