refactor(form): [form] refactor form style #2196
Conversation
WalkthroughThe changes in this pull request encompass modifications to form configurations across multiple files, including updates to default values, the addition of new properties, and the removal of deprecated components. Key alterations involve the introduction of an 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: 7
🧹 Outside diff range and nitpick comments (14)
packages/design/aurora/src/form/index.ts (3)
2-4: LGTM! Consider updating documentation.The addition of the
iconsobject with avalidateIconproperty is a good enhancement for form styling configuration. This aligns well with the PR objective of refactoring form style.Consider updating the component's documentation to explain the purpose and usage of the
validateIconproperty. This will help developers understand how to utilize this new configuration option effectively.
9-9: LGTM! Consider a minor naming improvement.The addition of the
hideRequiredAsteriskproperty enhances form styling flexibility, which aligns well with the PR objective.Consider renaming this property to
showRequiredAsteriskand setting its default value totrue. This would make the property name more intuitive (affirmative rather than negative) and maintain the same default behavior. The new code would look like this:- hideRequiredAsterisk: false, + showRequiredAsterisk: true,This change would make the code more readable and maintain the current default behavior.
10-10: LGTM! Consider adding documentation and improving type safety.The addition of the
messageTypeproperty enhances form styling flexibility, which aligns well with the PR objective.Consider the following improvements:
- Add documentation to explain the purpose of
messageTypeand its possible values.- Use a string literal type to improve type safety and developer experience. For example:
messageType: '' as '' | 'error' | 'warning' | 'success'This change would provide better autocomplete suggestions and catch potential typos at compile-time.
packages/design/saas/src/form/index.ts (2)
9-9: LGTM. Consider updating documentation.The addition of
hideRequiredAsteriskwith a default value offalseis a good choice. It allows users to optionally hide the asterisks for required fields while keeping them visible by default.Consider updating the component's documentation to reflect this new configuration option, explaining its purpose and usage.
2-10: Overall, good additions. Consider enhancing PR description.The changes introduce new configuration options (
icons,hideRequiredAsterisk,messageType) that enhance the form component's flexibility. These additions align with the PR objective of refactoring the form style by providing more control over the form's appearance.To improve the PR's clarity and facilitate easier review:
- Consider updating the PR description to explain the specific improvements these new configuration options bring.
- Provide examples of how these new options can be used to enhance form styling or behavior.
- If applicable, include before/after screenshots to visually demonstrate the impact of these changes.
packages/renderless/src/form/index.ts (1)
Line range hint
1-80: Summary of form style refactoring changesThe changes in this file primarily focus on refactoring the default behaviors of various form properties:
- Required asterisks are now hidden by default.
- A default error icon is now set.
- The
computedIsErrorInlinefunction has been simplified.- Error messages are now displayed as blocks by default.
While these changes align with the PR objective and generally improve the code, they may have significant impacts on existing form implementations. It's crucial to thoroughly test these changes across various scenarios to ensure they don't introduce unexpected visual or functional regressions.
Consider the following recommendations:
- Update the component documentation to reflect these new default behaviors.
- Create or update visual regression tests to capture any layout changes resulting from these modifications.
- Review and update any affected unit tests to account for the new default behaviors.
packages/vue/src/form-item/src/pc.vue (1)
41-42: LGTM: New component registered correctly.The
IconErrorcomponent is properly registered using the importediconErrorfunction. The naming convention follows Vue best practices.Consider adding a comment explaining the purpose of the
IconErrorcomponent for better code readability:components: { LabelWrap, Tooltip, + // IconError: Used for displaying error icons in the form IconError: iconError() },examples/sites/demos/apis/form.js (1)
Line range hint
1-624: Summary of Form API changesThe changes to the Form API documentation improve clarity, flexibility, and default behaviors. Key points:
- The
hide-required-asterisknow defaults totrue.- The default
label-widthhas been slightly increased to '84px'.- The
message-typeoption has been enhanced with more flexibility and a new default value.These changes, while beneficial, may impact existing implementations.
The updates to the API documentation are generally improvements.
Please ensure that all these changes are properly documented in the changelog and that the main documentation is updated to reflect these new defaults and options.
packages/theme/src/form/index.less (1)
30-30: Inconsistent CSS variable prefixes.There is an inconsistency in the prefixes of CSS variables used:
- Line 30:
--ti-Form-label-top-margin-bottom- Line 38:
--tv-Form-icon-color-error- Line 40:
--tv-Form-icon-sizeFor consistency and maintainability, consider standardizing the CSS variable prefixes to use either
--ti-or--tv-throughout the file.Also applies to: 38-38, 40-40
packages/theme/src/form-item/vars.less (1)
14-53: Consider using English for code comments to improve maintainabilityThe code comments are currently written in Chinese. To enhance readability and collaboration among international team members, it's recommended to use English for code comments.
packages/theme/src/form-item/index.less (4)
Line range hint
17-30: Ensure Consistent Variable Naming ConventionsThe introduction of prefix variables like
@form-prefix-cls,@input-prefix-cls, etc., is a good practice for maintaining consistency. Please verify that this naming convention aligns with the project's standards and is consistent across the codebase.
75-76: Address Pending TODO CommentThere's a TODO comment indicating that the
font-familystyle should be removed after global font styles are added. To maintain code cleanliness, it's advisable to resolve TODOs promptly.Would you like assistance in adding the global font styles so this inline style can be removed? I can help create a solution or open a GitHub issue to track this task.
127-129: Ensure Consistent Application of Font Sizes and Line HeightsVariables like
--tv-FormItem-font-sizeand--tv-FormItem-line-heightare used across multiple elements. Verify that these variables are consistently applied to maintain a uniform appearance throughout the form components.Also applies to: 138-152, 169-169
259-263: Review Spacing for Extra Tip SectionThe
.form-item__extra-tipclass has amargin-top: 8px. Confirm that this spacing aligns with the overall design system and doesn't introduce unintended gaps or misalignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- examples/sites/demos/apis/form.js (3 hunks)
- packages/design/aurora/src/form/index.ts (1 hunks)
- packages/design/saas/src/form/index.ts (1 hunks)
- packages/design/smb/index.ts (0 hunks)
- packages/design/smb/src/form/index.ts (0 hunks)
- packages/renderless/src/form/index.ts (3 hunks)
- packages/renderless/src/form/vue.ts (1 hunks)
- packages/theme/src/form-item/index.less (9 hunks)
- packages/theme/src/form-item/old-theme.js (1 hunks)
- packages/theme/src/form-item/vars.less (1 hunks)
- packages/theme/src/form/aurora-theme.js (1 hunks)
- packages/theme/src/form/index.less (1 hunks)
- packages/theme/src/form/old-theme.js (1 hunks)
- packages/theme/src/form/vars.less (1 hunks)
- packages/vue/src/form-item/src/pc.vue (2 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- packages/design/smb/index.ts
- packages/design/smb/src/form/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/theme/src/form/aurora-theme.js
- packages/theme/src/form/old-theme.js
🔇 Additional comments (17)
packages/design/aurora/src/form/index.ts (1)
1-11: Overall, good refactoring of form styling configuration.The changes introduced in this file align well with the PR objective of refactoring form style. The new properties (
icons,hideRequiredAsterisk, andmessageType) enhance the form component's configurability, allowing for more flexible styling options.A few minor suggestions have been made to improve naming, documentation, and type safety. These improvements would further enhance the developer experience and code maintainability.
Great work on this refactoring effort!
packages/design/saas/src/form/index.ts (2)
2-4: Clarify the purpose ofvalidateIconand consider a meaningful default.The addition of the
iconsproperty withvalidateIconis noted, but its purpose and usage are not clear from the context.Could you provide more information on how
validateIconis used in the form component?Also, consider setting a more meaningful default value for
validateIconinstead of an empty string, if applicable. This could improve the out-of-the-box experience for developers using this configuration.
10-10: Clarify the purpose and consider a more specific type formessageType.The addition of the
messageTypeproperty is noted, but its purpose and possible values are not clear from the context.Could you provide more information on how
messageTypeis used in the form component and what values it can take?Consider using a more specific type (like a string union type in TypeScript) or a meaningful default value instead of an empty string. This could improve type safety and make the intended use clearer.
packages/theme/src/form/vars.less (1)
14-17: 🛠️ Refactor suggestionNew icon-related variables added
The addition of
--tv-Form-icon-sizeand--tv-Form-icon-color-errorvariables is a good step towards more consistent and maintainable icon styling in forms. These variables use predefined values, which promotes consistency across the theme.However, I have a few observations:
The naming convention has changed from
--ti-to--tv-. Is this part of a larger refactoring effort? If so, ensure this change is consistent across all files and documented.Several variables related to form validation and layout have been removed. Please confirm that these are no longer needed and that their removal doesn't negatively impact the form's functionality or appearance.
The comments are in Chinese. Consider using English comments for better international collaboration, or implement a localization strategy for comments if multilingual support is a project requirement.
Consider adding brief English translations alongside the Chinese comments:
- // 表单图标大小 + // 表单图标大小 (Form icon size) --tv-Form-icon-size: var(--tv-icon-size); - // 表单错误提示图标颜色 + // 表单错误提示图标颜色 (Form error icon color) --tv-Form-icon-color-error: var(--tv-color-error-icon);To ensure consistency in variable naming and usage across the project, run the following script:
packages/renderless/src/form/vue.ts (1)
84-84: Approved change, but clarification and documentation needed.The update of the default
labelWidthfrom '80px' to '84px' aligns with the PR objective of refactoring the form style. However, I have a few suggestions:
Could you please clarify the reasoning behind changing the default width to 84px specifically? Understanding the motivation will help in reviewing the change more effectively.
It's recommended to document this change in the component's documentation, as it might affect the layout of forms using the default label width.
Please verify the impact of this change on existing forms that rely on the default label width. Consider adding before/after screenshots to the PR description to visualize the improvement.
To help assess the impact of this change, you can run the following script to find all usages of the Form component without an explicit labelWidth prop:
This will help identify forms that might be affected by the default labelWidth change.
packages/renderless/src/form/index.ts (3)
68-68: Simplification ofcomputedIsErrorInlineis correct.The removal of
|| falseis a good simplification. The function will still returnfalseifdesignConfig?.messageTypeis not'inline', maintaining the same behavior as before.
56-56: Verify the impact of changing the default validate icon.The default value for
validateIconhas been changed fromnullto'icon-error'. This means that an error icon will be displayed by default unless explicitly set otherwise. Please ensure this aligns with the intended design and doesn't introduce unexpected visual changes in existing forms.To check the impact, run the following script:
#!/bin/bash # Search for usages of validateIcon prop rg --type vue 'validateIcon'
77-80: Verify the impact of changing the default error display to block.The
computedIsErrorBlockfunction has been updated with two significant changes:
- It now uses
Object.hasOwnProperty.call()to safely check for the existence ofmessageTypeindesignConfig.- The default return value has been changed to
true, meaning error messages will be displayed as blocks by default.These changes improve the safety of the property check and alter the default error message display behavior. Please ensure this aligns with the intended design and doesn't introduce unexpected layout changes in existing forms.
To check the impact, run the following script:
#!/bin/bash # Search for usages of messageType prop or inlineMessage prop rg --type vue 'messageType|inlineMessage'packages/vue/src/form-item/src/pc.vue (2)
28-28: LGTM: New import statement added correctly.The import of
iconErrorfrom@opentiny/vue-iconis properly placed and follows the correct naming convention.
28-28: Verify the usage of the new IconError component.The changes successfully add the
IconErrorcomponent to the form item, which aligns with the PR objective of refactoring the form style. The implementation is consistent with Vue best practices and doesn't introduce any breaking changes.To ensure the
IconErrorcomponent is being used correctly, please run the following script:If the script doesn't find any usage of
IconErrorin the template, consider adding it to the appropriate place in the form item's render function.Also applies to: 41-42
✅ Verification successful
Re-run the verification scripts without specifying the file type.
To address the previous errors with unrecognized file types, please execute the following script:
The usage of the
IconErrorcomponent has been successfully verified.The
IconErrorcomponent is correctly integrated into the form item without any related TODOs. The implementation aligns with the PR objective and adheres to Vue best practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of IconError component in the form item template. # Test: Search for IconError usage in the template. Expect: At least one occurrence. rg --type vue 'IconError' packages/vue/src/form-item/src/pc.vue # Test: Check if there are any TODO comments related to IconError. Expect: No occurrences. rg --type vue 'TODO.*IconError' packages/vue/src/form-item/src/pc.vueLength of output: 191
Script:
#!/bin/bash # Description: Verify the usage of IconError component in the form item template without specifying file type. # Test: Search for IconError usage in the template. Expect: At least one occurrence. rg 'IconError' packages/vue/src/form-item/src/pc.vue # Test: Check if there are any TODO comments related to IconError. Expect: No occurrences. rg 'TODO.*IconError' packages/vue/src/form-item/src/pc.vueLength of output: 140
examples/sites/demos/apis/form.js (2)
134-141: Enhancements tomessage-typeoption
- The
message-typenow includes 'absolute' as a valid type, providing more flexibility in positioning error messages.- The default value has been changed from an empty string to 'block', making the behavior more predictable.
- The description has been updated to clarify the behavior when
validate-typeis set totext.These changes improve the flexibility and clarity of the
message-typeoption. The new default value of 'block' may affect existing implementations that relied on the previous default behavior.Let's check for any existing usage of
message-typethat might be affected:#!/bin/bash # Search for existing usage of message-type rg --type js --type vue 'message-type'
122-122: Minor adjustment to defaultlabel-widthThe default value for
label-widthhas been changed from '80px' to '84px'.This change is likely for design consistency or to accommodate longer labels. However, it would be helpful to understand the reasoning behind this specific adjustment.
To ensure this change doesn't negatively impact existing forms, let's check for any hardcoded widths:
packages/theme/src/form/index.less (1)
34-44: Refactored validation styles enhance maintainability.The consolidation of validation styles under
.@{form-prefix-cls}__validsimplifies the stylesheet and improves readability. The use of nested selectors is effective and aligns with best practices.packages/theme/src/form-item/vars.less (1)
52-53: Verify themargin-bottomvalue for the 'xs' sizeThe variable
--tv-FormItem-margin-bottom-xsis set to20px, while other sizes (md,lg,sm) have amargin-bottomof24px. Please confirm if this difference is intentional.packages/theme/src/form-item/index.less (3)
Line range hint
31-60: Review Line-Height and Height AlignmentIn the
.size-height(@height, @margin-bottom)mixin,line-heightis set to@heightfor various elements. Ensure that using the same value for bothheightandline-heightdoes not cause vertical alignment issues, especially with different font sizes or when content wraps.
198-206: Verify Error Styling VariablesThe error styles now use variables such as
var(--tv-FormItem-text-color-error)andvar(--tv-FormItem-icon-color-error). Please confirm that these variables are defined and conform to the design specifications for error states.Also applies to: 212-222
236-242: Ensure Error State Styles Meet Accessibility StandardsThe updated error state styles for inputs and textareas change
border-colorandbackground-color. Verify that these color changes provide sufficient contrast ratios as per accessibility guidelines.Also applies to: 254-255
| 'ti-FormItem-margin-bottom-medium': 'var(--ti-common-space-5x, 20px)', | ||
| 'ti-FormItem-margin-bottom-small': 'var(--ti-common-space-5x, 20px)', | ||
| 'ti-FormItem-margin-bottom-mini': 'var(--ti-common-space-4x, 16px)', | ||
| 'ti-FormItem-margin-bottom-default': 'var(--ti-common-space-5x, 20px)', | ||
| 'ti-FormItem-input-medium-height': 'var(--ti-common-size-10x, 40px)', | ||
| 'ti-FormItem-input-small-height': 'var(--ti-common-size-8x, 32px)', | ||
| 'ti-FormItem-input-mini-height': 'var(--ti-common-size-6x, 24px)', | ||
| 'ti-FormItem-label-padding-right': 'var(--ti-common-space-2x, 8px)', | ||
| 'ti-FormItem-mini-line-height': 'var(--ti-common-line-height-5, 24px)', | ||
| 'ti-FormItem-medium-line-height': 'calc(var(--ti-common-line-height-7, 36px) + 4px)', | ||
| 'ti-FormItem-small-line-height': 'var(--ti-common-line-height-6, 32px)' |
There was a problem hiding this comment.
💡 Codebase verification
Old CSS variable names still present in the codebase.
The search revealed that the following old CSS variable names are still in use:
ti-form-item-error-text-colorinpackages/theme/src/theme/infinity-theme/component-vars.jsti-form-item-error-bg-colorinpackages/theme/src/theme/infinity-theme/component-vars.js
These instances indicate that not all old CSS variable names have been updated, which may lead to inconsistencies and potential styling issues.
🔗 Analysis chain
Consistent renaming of CSS variables improves code style.
The refactoring of CSS variable names from kebab-case to PascalCase for the component name part (e.g., form-item to FormItem) improves consistency with Vue component naming conventions. This change enhances code readability and maintainability.
To ensure that these changes don't break existing code, please run the following script to check for any remaining usage of the old variable names:
Consider adopting full PascalCase for the entire variable name, including the 'ti' prefix, for complete consistency. For example:
- 'ti-FormItem-margin-bottom-medium'
+ 'Ti-FormItem-margin-bottom-medium'While this change improves consistency, it potentially introduces a breaking change for consumers of this theme. Consider the following:
- If this is a public API, ensure that this change is communicated clearly in the changelog and documentation.
- Consider providing a migration guide or deprecation warnings if backwards compatibility is a concern.
- Update any documentation that references these CSS variables to reflect the new naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of old CSS variable names
# Test: Search for old variable names. Expect: No results
rg --type css --type vue --type js 'ti-form-item-'
Length of output: 78
Script:
#!/bin/bash
# Description: Check for usage of old CSS variable names in .css, .js, and .vue files
# Search for old variable names. Expect: No results
rg 'ti-form-item-' --glob '*.css' --glob '*.js' --glob '*.vue'
Length of output: 289
| @@ -47,13 +47,13 @@ export const computedAutoLabelWidth = | |||
| export const computedHideRequiredAsterisk = | |||
| ({ props, designConfig }: Pick<IFormRenderlessParams, 'props' | 'designConfig'>) => | |||
| (): boolean => { | |||
| return props.hideRequiredAsterisk ?? designConfig?.hideRequiredAsterisk ?? false | |||
| return props.hideRequiredAsterisk ?? designConfig?.hideRequiredAsterisk ?? true | |||
There was a problem hiding this comment.
💡 Codebase verification
Impact of Changing Default Behavior for Hiding Required Asterisks
Changing the default value of hideRequiredAsterisk from false to true affects multiple components across different packages. This alteration will hide required field indicators by default unless explicitly overridden. Please review the following files to ensure this change aligns with the intended design and does not adversely affect form usability:
packages/vue/src/form/src/index.tspackages/renderless/types/form-item.type.tspackages/renderless/types/form.type.tspackages/renderless/src/form/vue.tspackages/renderless/src/form-item/vue.tspackages/renderless/src/form/index.tspackages/design/saas/src/form/index.tspackages/design/aurora/src/form/index.ts
🔗 Analysis chain
Verify the impact of changing the default behavior for hiding required asterisks.
The default behavior for hideRequiredAsterisk has been changed from false to true. This means that required field indicators will be hidden by default unless explicitly set. Please ensure this aligns with the intended design and doesn't negatively impact form usability.
To check the impact, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of hideRequiredAsterisk prop
rg --type vue 'hideRequiredAsterisk'
Length of output: 71
Script:
#!/bin/bash
# Search for usages of hideRequiredAsterisk prop in TypeScript files
rg --type typescript 'hideRequiredAsterisk'
Length of output: 78
Script:
#!/bin/bash
# Search for usages of hideRequiredAsterisk prop in TypeScript files
rg 'hideRequiredAsterisk' --glob '*.ts' --glob '*.tsx'
Length of output: 770
| defaultValue: 'true', | ||
| desc: { | ||
| 'zh-CN': '是否隐藏必填字段的标签旁边的红色星号，SMB 主题下默认值为true', | ||
| 'en-US': | ||
| 'Whether to hide the red asterisk next to the label of mandatory fields, The default value for SMB theme is true' | ||
| 'zh-CN': '是否隐藏必填字段的标签旁边的红色星号', | ||
| 'en-US': 'Whether to hide the red asterisk next to the label of mandatory fields' |
There was a problem hiding this comment.
Significant change in default behavior for hide-required-asterisk
The default value for hide-required-asterisk has been changed from false to true. This means that by default, the red asterisk next to mandatory fields will now be hidden unless explicitly set to false.
The description update improves clarity and internationalization.
Please ensure that this change in default behavior is intentional and documented in the changelog, as it may affect existing implementations.
The updated description is clear and concise.
| .star-selector(@content) { | ||
| &.is-required:not(.is-no-asterisk) { | ||
| // 子选择器是避免影响嵌套表单场景 | ||
| .@{form-item-prefix-cls}__label-wrap > .@{form-item-prefix-cls}__label:before, | ||
| & > .@{form-item-prefix-cls}__label:before { | ||
| @content(); | ||
| } |
There was a problem hiding this comment.
Assess Accessibility of .star-selector Mixin
The .star-selector(@content) mixin adds a visual indicator for required fields. Please ensure that this method does not impact accessibility, and consider whether additional ARIA attributes are needed for screen readers.
| .size-height( | ||
| var(--tv-FormItem-height), | ||
| var(--tv-FormItem-margin-bottom) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Simplifying Repetitive Mixin Calls
The .size-height mixin is repeatedly called with different parameters for various size modifiers (--medium, --small, --mini). To reduce repetition, consider parameterizing these calls or consolidating size variables.
Also applies to: 94-95, 101-102, 108-109
| .star-selector({ | ||
| content: '*'; | ||
| color: var(--tv-FormItem-text-color-error); | ||
| margin-right: 4px; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Enhance Accessibility for Required Field Indicators
The use of content: '*' in the .star-selector mixin visually denotes required fields. To improve accessibility, ensure that required fields are also programmatically indicated using attributes like aria-required="true".
| vertical-align: top; | ||
| margin-right: 24px; | ||
| } | ||
|
|
||
| .@{form-item-prefix-cls}__label { | ||
| float: none; | ||
| display: inline-block; | ||
| } | ||
|
|
||
| .@{form-item-prefix-cls}__content { | ||
| display: inline-block; | ||
| vertical-align: top; | ||
| } | ||
|
|
||
| // 嵌套表单场景 | ||
| .@{form-prefix-cls}--label-top .@{form-item-prefix-cls}__content { | ||
| display: block; | ||
| } | ||
| } | ||
|
|
||
| &--label-left { | ||
| &.has-required { | ||
| .@{form-item-prefix-cls}__label { | ||
| padding-left: 12px; | ||
| } | ||
| } | ||
| .@{form-item-prefix-cls}__label { | ||
| text-align: left; | ||
| } | ||
|
|
||
| &.label-align { | ||
| .@{form-item-prefix-cls} { | ||
| &.is-required { | ||
| .@{form-item-prefix-cls}__label { | ||
| padding-left: 12px; | ||
| position: relative; | ||
|
|
||
| &::before { | ||
| position: absolute; | ||
| left: 0; | ||
| } | ||
| } | ||
| & ~ .@{form-item-prefix-cls} { | ||
| .@{form-item-prefix-cls}__label { | ||
| padding-left: 12px; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &--label-top { | ||
| .@{form-item-prefix-cls} { | ||
| margin-bottom: 16px; | ||
| } | ||
| .@{form-item-prefix-cls}__label { | ||
| float: none; | ||
| display: block; | ||
| text-align: left; | ||
| padding: 0 0 8px; | ||
| height: auto; | ||
| line-height: var(--tv-FormItem-line-height); | ||
| } | ||
| } | ||
|
|
||
| &.is-display-only { | ||
| .@{form-item-prefix-cls}__label { | ||
| line-height: var(--tv-FormItem-line-height); | ||
| height: auto; | ||
| vertical-align: inherit; | ||
| color: var(--tv-FormItem-text-color-weaken); | ||
| } | ||
|
|
||
| .@{form-item-prefix-cls}__content { | ||
| line-height: var(--tv-FormItem-line-height); | ||
| font-size: var(--tv-FormItem-font-size); | ||
| height: auto; | ||
| vertical-align: inherit; | ||
|
|
||
| // 一些组件的只读态实现方式怪异，需要一些修正样式来修复水平对齐问题 | ||
| .@{switch-prefix-cls} { | ||
| margin-top: 0; | ||
| } | ||
|
|
||
| .@{css-prefix}numeric.is-display-only { | ||
| margin-top: 2px; | ||
| } | ||
| .@{input-prefix-cls} { | ||
| &.@{range-editor-prefix-cls}.@{input-prefix-cls}__inner { | ||
| height: 18px; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .@{form-item-prefix-cls} { | ||
| .star-selector({ | ||
| content: ''; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Complex Nested Selectors for Better Maintainability
Within the .@{form-prefix-cls} block, there are deeply nested selectors for different form states (&--inline, &--label-left, &--label-top, etc.). Consider refactoring these into separate, more manageable blocks or using mixins to improve readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
examples/sites/demos/pc/app/form/message-type.spec.ts (1)
16-19: LGTM! Consider adding comments for improved readability.The changes to the test case look good. The new order of button clicks and assertions covers all three types of error message positioning (block, inline, and absolute) in a logical sequence. The assertions correctly match the expected behavior for each button click.
To improve code readability, consider adding brief comments before each button click to explain the purpose of each test case. For example:
// Test block-level display await getBtnByText('块级').click() await expect(firstItemError).toHaveCSS('display', 'flex') // Test inline display await getBtnByText('行内').click() await expect(firstItemError).toHaveCSS('display', 'inline-block') // Test absolute positioning await getBtnByText('绝对定位').click() await expect(firstItemError).toHaveCSS('position', 'absolute')This addition would make the test's structure and purpose even clearer at a glance.
examples/sites/demos/pc/app/form/label-align-composition-api.vue (3)
4-10: LGTM: Addition of:hide-required-asterisk="false"propThe addition of the
:hide-required-asterisk="false"prop to the<tiny-form>component is correct and consistent with the component's existing structure. This change explicitly sets the visibility of required field asterisks, which can improve form clarity for users.A few observations:
- The new prop aligns well with the form's purpose of demonstrating label alignment options.
- The
falsevalue ensures that required field asterisks are shown, which is generally a good practice for form usability.- The prop is correctly placed within the component's attribute list.
Consider adding a comment explaining the purpose of this prop, especially if it's not a commonly used option. For example:
<tiny-form ref="ruleFormRef" :model="createData" label-position="left" :label-align="isLabelAlign" :hide-required-asterisk="false" <!-- Ensures required field asterisks are visible --> >
Line range hint
11-19: Enhance password field security and user experienceWhile the form structure looks good, consider improving the password fields for better security and user experience:
- Add
autocomplete="new-password"to both password fields to prevent browsers from auto-filling them with saved passwords.- Consider adding a password strength indicator for the main password field.
- For the "密钥" (password2) field, add a validation to ensure it matches the main password if it's intended as a confirmation field.
Here's an example of how you could enhance the password fields:
<tiny-form-item label="密码" prop="password" required> <tiny-input v-model="createData.password" type="password" show-password autocomplete="new-password" ></tiny-input> <!-- Add a password strength indicator component here --> </tiny-form-item> <tiny-form-item label="确认密码" prop="password2" required> <tiny-input v-model="createData.password2" type="password" show-password autocomplete="new-password" ></tiny-input> </tiny-form-item>
Line range hint
4-23: Add form validation logicThe form currently lacks validation logic, which is crucial for ensuring data integrity and providing user feedback. Consider adding form validation rules and handling form submission.
Here's an example of how you could add basic form validation:
- Add validation rules in the
<script setup>section:import { reactive } from 'vue' const rules = reactive({ username: [ { required: true, message: '请输入用户名', trigger: 'blur' }, { min: 3, max: 20, message: '长度在 3 到 20 个字符', trigger: 'blur' } ], password: [ { required: true, message: '请输入密码', trigger: 'blur' }, { min: 6, message: '密码长度不能小于 6 个字符', trigger: 'blur' } ], password2: [ { required: true, message: '请确认密码', trigger: 'blur' }, { validator: (rule, value, callback) => { if (value !== createData.password) { callback(new Error('两次输入密码不一致!')) } else { callback() } }, trigger: 'blur' } ] })
Add the
:rules="rules"prop to the<tiny-form>component.Implement a submit method:
const submitForm = async () => { if (!ruleFormRef.value) return try { await ruleFormRef.value.validate() // Handle successful validation (e.g., API call) console.log('Form submitted successfully') } catch (error) { console.error('Form validation failed', error) } }
- Add a submit button to the form:
<tiny-form-item> <tiny-button type="primary" @click="submitForm">提交</tiny-button> </tiny-form-item>These changes will significantly improve the form's functionality and user experience.
examples/sites/demos/pc/app/form/label-align.vue (1)
4-10: LGTM! Consider a minor improvement for clarity.The addition of
:hide-required-asterisk="false"to the<tiny-form>component is correct and aligns with the PR objective of refactoring the form style. This change explicitly ensures that required field indicators are shown.For improved readability, consider using the positive form of the prop:
- :hide-required-asterisk="false" + :show-required-asterisk="true"This change would make the intent clearer at a glance, assuming the
tiny-formcomponent supports this prop name.examples/sites/demos/pc/app/form/message-type-composition-api.vue (2)
52-52: Improved clarity in messageTypeList optionsThe change from
{ text: '默认', value: '' }to{ text: '块级', value: 'block' }is a good improvement. It removes the ambiguous empty string value and provides a more descriptive option that aligns with the actual behavior.Consider using consistent naming between the text and value properties. For example:
{ text: 'Block', value: 'block' }This would improve readability and maintain consistency across different language settings.
49-54: Summary of changes and suggestionsThe changes in this file improve the form component's message type functionality by:
- Setting a default message type of 'block'.
- Clarifying the options in
messageTypeList.- Adding a new 'absolute' positioning option.
These changes enhance the component's usability and flexibility. To further improve the code:
- Consider adding comments explaining the purpose of each message type option.
- Update the component's documentation to reflect these changes, especially the new 'absolute' positioning option.
- If not already present, add unit tests to cover the different message type behaviors.
Consider extracting the
messageTypeListto a separate constants file if it's used across multiple components. This would improve maintainability and ensure consistency across the application.examples/sites/demos/pc/app/form/message-type.vue (1)
53-53: LGTM: Updated label for 'block' message typeThe change from '默认' (default) to '块级' (block-level) for the 'block' value is more descriptive and aligns well with the new default
messageType. This improves clarity for users.Consider adding a comment explaining the meaning of each message type, especially for non-Chinese speakers who might maintain this code in the future. For example:
messageTypeList: [ { text: '块级', value: 'block' }, // Block-level messages { text: '行内', value: 'inline' }, // Inline messages { text: '绝对定位', value: 'absolute' } // Absolutely positioned messages ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- examples/sites/demos/pc/app/form/label-align-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/form/label-align.vue (1 hunks)
- examples/sites/demos/pc/app/form/message-type-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/form/message-type.spec.ts (1 hunks)
- examples/sites/demos/pc/app/form/message-type.vue (1 hunks)
- examples/sites/demos/pc/app/form/size.spec.ts (1 hunks)
- examples/sites/demos/pc/app/form/slot-label.spec.ts (1 hunks)
- examples/sites/demos/pc/app/form/smb-required.spec.ts (0 hunks)
- packages/design/aurora/src/form/index.ts (1 hunks)
- packages/design/saas/src/form/index.ts (1 hunks)
- packages/theme/src/form-item/index.less (9 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- examples/sites/demos/pc/app/form/smb-required.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/design/aurora/src/form/index.ts
- packages/design/saas/src/form/index.ts
- packages/theme/src/form-item/index.less
🔇 Additional comments (9)
examples/sites/demos/pc/app/form/slot-label.spec.ts (1)
13-13: LGTM! Consider verifying visual appearance.The change in the expected text for the third form item label aligns with the PR objective of refactoring the form style. The new text '超过两行文字，省略显示' (which translates to "More than two lines of text, ellipsis display") suggests an improvement in handling longer label text.
To ensure the visual changes meet expectations, please verify:
- The form correctly displays the new, longer label text.
- The ellipsis is applied appropriately when the text exceeds two lines.
- The overall layout of the form remains consistent and visually appealing with this change.
Consider adding a visual regression test if not already in place.
examples/sites/demos/pc/app/form/label-align.vue (1)
Line range hint
1-55: Summary: Successful refactoring of form styleThe changes in this file successfully implement the PR objective of refactoring the form style. The addition of the
:hide-required-asterisk="false"prop to the<tiny-form>component enhances the form's configuration options by explicitly showing required field indicators.Key points:
- The change is minimal and focused, affecting only the template section.
- No modifications were needed in the script or style sections.
- The change is consistent with the AI-generated summary and doesn't introduce any breaking changes.
Overall, this refactoring improves the clarity of the form's behavior regarding required fields.
examples/sites/demos/pc/app/form/message-type-composition-api.vue (2)
49-49: Improved default state for messageTypeThe initialization of
messageTypewith 'block' is a good change. It ensures that the form always has a valid message type, even before user interaction. This aligns well with the updates in themessageTypeListand provides a better default state for the component.
54-54: New option for absolute positioning of error messagesThe addition of
{ text: '绝对定位', value: 'absolute' }tomessageTypeListis a good enhancement. It expands the functionality of the form component by allowing more flexible error message placement.To ensure this new option is properly documented and implemented, please run the following script:
Please ensure that the new 'absolute' option is properly documented and implemented in the form component. If necessary, update the documentation and add appropriate test cases for this new functionality.
examples/sites/demos/pc/app/form/size.spec.ts (2)
Line range hint
1-52: Overall assessment: LGTM with minor verification needed.The change in this file is consistent with the PR objective of refactoring the form style. The modification of the 'small' form element height from 32px to 28px maintains a logical progression of sizes (medium: 40px, small: 28px, mini: 24px).
The test case structure remains intact, and the change is applied consistently within the file. However, it's important to ensure this change is reflected across the entire codebase and documentation.
26-26: Verify the new 'small' form element height.The height for 'small' form elements has been changed from 32px to 28px. This adjustment affects the expected height for the submit button, input field, and label when the 'small' size is selected.
- Please confirm that this change accurately reflects the new design specifications for 'small' form elements.
- Ensure that this change is consistent across all components that use the 'small' size.
- Update any related documentation or design guidelines to reflect this new height value for 'small' form elements.
To verify this change across the codebase, you can run the following script:
This script will help identify any inconsistencies or places where the old value might still be used.
✅ Verification successful
'small' form element height change verified successfully.
No inconsistencies found with the new height value of
28pxfor 'small' form elements across the codebase. All references have been updated accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other occurrences of form element heights # and verify consistency with the new 'small' size value. echo "Searching for 'small' size definitions in .vue and .ts files:" rg --type vue --type typescript -i 'small.*height.*32px' -g '!examples/' echo "Searching for form item height definitions:" rg --type vue --type typescript -i 'form-item.*height' -g '!examples/'Length of output: 439
Script:
#!/bin/bash # Description: Search for other occurrences of form element heights # and verify consistency with the new 'small' size value by targeting specific file extensions. echo "Searching for 'small' size definitions in .vue and .ts files:" rg -i 'small.*height.*32px' --glob '*.vue' --glob '*.ts' -g '!examples/' echo "Searching for form item height definitions in .vue and .ts files:" rg -i 'form-item.*height' --glob '*.vue' --glob '*.ts' -g '!examples/'Length of output: 427
examples/sites/demos/pc/app/form/message-type.vue (3)
50-50: LGTM: Default message type set to 'block'Setting a default value for
messageTypeis a good practice. It ensures that the form always has a defined message type, improving predictability and potentially avoiding undefined behavior.
50-55: Overall assessment: Good refactoring of form stylesThe changes made to this file improve the form component by:
- Setting a clear default message type.
- Providing more descriptive labels for message type options.
- Adding a new 'absolute' positioning option for increased flexibility.
These modifications enhance both the usability and maintainability of the form component. Good job on this refactoring effort!
55-55: LGTM: Added 'absolute' positioning option for messagesThe addition of an 'absolute' positioning option for messages increases the flexibility of the form component. This is a good enhancement that allows for more diverse layouts.
To ensure this new option is properly implemented, please verify:
- The form component correctly handles the 'absolute' value for
message-type.- There's appropriate CSS to style absolutely positioned messages.
- The documentation is updated to reflect this new option.
You can use the following script to check for related CSS and documentation updates:
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
Release Notes
New Features
hideRequiredAsterisk,labelWidth, and expandedmessageType.Bug Fixes
Style
Chores