refactor(alert): [alert] refactor Alert's css vars #2240
Conversation
WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 6
🧹 Outside diff range and nitpick comments (15)
examples/sites/demos/pc/app/alert/slot-default-composition-api.vue (1)
49-51: Consider using a CSS variable for the margin valueThe addition of this CSS rule improves the spacing between action links in the alert components, which is a good enhancement. However, to improve maintainability and consistency across the project, consider using a CSS variable for the margin value instead of a hard-coded pixel value.
Here's a suggested modification:
+ :root { + --tiny-alert-action-link-margin: 16px; + } .demo-alert .tiny-alert__opration a { - margin-right: 16px; + margin-right: var(--tiny-alert-action-link-margin); }This approach allows for easier global adjustments and maintains consistency across the component library.
examples/sites/demos/pc/app/alert/slot-default.vue (1)
12-17: LGTM! Consistent styling and improved readability.The addition of
class="demo-alert"to multiple alert components enhances styling consistency. The restructuring of attributes improves code readability. These changes align well with the PR's refactoring objective.Consider adding
class="demo-alert"to the first twotiny-alertcomponents as well for complete consistency across all examples.Also applies to: 22-27, 29-34
examples/sites/demos/pc/app/alert/webdoc/alert.js (7)
13-13: Improve readability of English descriptionThe addition of a line break (
<br>) in the Chinese description improves readability. Consider applying a similar change to the English description for consistency.'en-US': - '<p>Set the prompt content through the <code>description</code> attribute or <code>description</code> slot, and set different types through <code>type</code> . </p>' + '<p>Set the prompt content through the <code>description</code> attribute or <code>description</code> slot.<br>Set different types through <code>type</code>.</p>'
22-43: Excellent improvements to size mode documentationThe changes to the 'size' demo name and description are highly beneficial:
- The updated name "Size Mode" more accurately reflects the feature.
- The expanded description provides valuable information about the differences between size modes.
- The use of HTML formatting enhances readability and structure.
These improvements will greatly aid users in understanding and utilizing the size modes.
Consider a minor improvement in the English description:
- Use <code>size</code> to set different size modes, optional values: <code>normal</code>, <code>large</code>. <br> + Use the <code>size</code> prop to set different size modes. Optional values are <code>normal</code> and <code>large</code>.<br>
68-68: Improve consistency in 'center' demo descriptionThe change in the Chinese description from "文字" (text) to "内容" (content) more accurately reflects the component's behavior. To maintain consistency:
- Consider updating the English description similarly.
- Update the demo name in both languages to reflect "content" instead of "text".
Suggested changes:
name: { - 'zh-CN': '文字居中', - 'en-US': 'Center text' + 'zh-CN': '内容居中', + 'en-US': 'Center Content' }, desc: { 'zh-CN': '<p>通过 <code>center</code> 设置内容显示居中。</p>', - 'en-US': '<p>You can use the <code>center</code> property to center the text. </p>' + 'en-US': '<p>You can use the <code>center</code> property to center the content. </p>' },
81-81: Enhance English description for icon customizationThe updated Chinese description provides clearer and more detailed information about icon customization. To maintain consistency and completeness, consider updating the English description similarly.
Suggested change for the English description:
-'<p>By setting custom icons through <code>icon</code> , the corresponding icons will be automatically used by default based on different <code>type</code> values.</p>' +'<p>Use the <code>icon</code> property to set a custom icon. If not set, default icons will be automatically used based on different <code>type</code> values.</p>'
108-108: Enhance English description for show-icon propertyThe updated Chinese description provides more specific information about the
show-iconproperty, clarifying that it controls the visibility of the left icon. To maintain consistency and accuracy across languages:Consider updating the English description as follows:
-'<p>Set whether the left icon is displayed by <code>show icon</code> .</p>' +'<p>Use the <code>show-icon</code> property to control the visibility of the left icon.</p>'
120-132: Comprehensive improvements to close button documentationThe extensive updates to the 'custom-close' demo descriptions are highly beneficial:
- They provide detailed explanations of the
closableproperty,close-textproperty, andcloseslot.- The default behavior and customization options are clearly explained.
- The addition of HTML formatting enhances readability.
These improvements will greatly aid users in understanding and utilizing the close button functionality.
To ensure consistency between language versions, consider adding the note about the fade-out animation to the English description:
'en-US': ` Enable the built-in close icon with the <code>closable</code> property, which defaults to <code>true</code>. <br /> Set the close button text by <code>close-text</code> instead of the close icon, effective only if <code>closable</code> is <code>true</code>. <br /> Unblock the built-in closing function when <code>closable</code> is set to <code>false</code>. At this point, you can completely customize the presentation of the close button area via the <code>close</code> slot. + <div class="tip custom-block"> + <p class="custom-block-title">Note: The component has a fade-out animation when closed or hidden. See the example for details!</p> + </div> `
Line range hint
138-139: Correct the English name for the 'custom-class' demoThere appears to be an error in the English name for the 'custom-class' demo. It has been incorrectly changed to 'custom close button', which doesn't match the demo's purpose of setting a custom class name.
Please correct the English name as follows:
name: { 'zh-CN': '自定义类名', - 'en-US': 'custom close button' + 'en-US': 'Custom Class Name' },examples/sites/demos/apis/alert.js (2)
251-251: Approved with a minor suggestion.The updated description for the
defaultslot provides more detailed information, which is helpful. However, consider the following suggestion:
- For consistency, update the English description to match the new Chinese description.
- Use markdown backticks instead of HTML
<code>tags for inline code formatting, as it's more common in markdown-based documentation.Here's a suggested improvement:
'zh-CN': '组件默认插槽，当 size 设置为 large 时有效，显示在 <code>description</code>下方 ', -'en-US': 'The default slot for the component is valid when size is set to large' +'en-US': 'The default slot for the component is valid when size is set to large, displayed below the `description`'
264-264: Approved with a suggestion for consistency.The updated description for the
descriptionslot provides more clarity about its purpose. However, to maintain consistency across languages:
- Consider updating the English description to match the level of detail in the Chinese description.
Here's a suggested improvement:
'zh-CN': '提示内容插槽', -'en-US': 'Prompt Content' +'en-US': 'Slot for prompt content'packages/theme/src/alert/index.less (4)
23-23: Translate comments to English for better collaborationThe comment on line 23 is in Chinese:
border: none; // 规范中不要border,所以没有border的相关css变量了。For better maintainability and collaboration, please consider translating comments into English.
49-49: Translate comments to English for consistent documentationThe comment on line 49 is in Chinese:
// 由于close-icon是abs， 需要给出安全边距防止重叠Translating comments to English ensures that all team members can understand and maintain the code effectively.
65-65: Translate comments to English for better readabilityThe comment on line 65 is in Chinese:
// .is-custom 仅用于close按钮处, 等效于close场景Please consider providing comments in English to enhance code readability for all contributors.
97-97: Translate comments to English for universal understandingThe comment on line 97 is in Chinese:
// 由于close-icon是abs， 需要给出安全边距防止重叠Using English for comments helps ensure that the codebase is accessible to all team members.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/sites/demos/apis/alert.js (2 hunks)
- examples/sites/demos/pc/app/alert/slot-default-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/alert/slot-default.vue (2 hunks)
- examples/sites/demos/pc/app/alert/webdoc/alert.js (8 hunks)
- packages/theme/src/alert/index.less (1 hunks)
- packages/theme/src/alert/vars.less (1 hunks)
- packages/theme/src/base/transition.less (1 hunks)
- packages/theme/src/mixins/alert.less (0 hunks)
- packages/theme/src/transition/alert-fade.less (0 hunks)
💤 Files with no reviewable changes (2)
- packages/theme/src/mixins/alert.less
- packages/theme/src/transition/alert-fade.less
🧰 Additional context used
🔇 Additional comments (9)
examples/sites/demos/pc/app/alert/slot-default-composition-api.vue (3)
12-17: LGTM: Improved formatting and consistent stylingThe changes enhance readability by formatting the
<tiny-alert>component across multiple lines. Addingclass="demo-alert"ensures consistency with other alert components and likely relates to the new CSS rule. These modifications improve the overall structure and maintainability of the code.
22-27: LGTM: Consistent formatting and stylingThe changes in this segment mirror the improvements made to the previous alert component. The multi-line formatting enhances readability, and the addition of
class="demo-alert"maintains styling consistency across all alert components in the file.
29-34: LGTM: Consistent improvements across all alert componentsThe changes in this segment complete the pattern of improvements applied to all alert components in the file. The multi-line formatting and addition of
class="demo-alert"enhance readability and ensure styling consistency. This uniform approach across all components is commendable, as it improves the overall structure and maintainability of the code.examples/sites/demos/pc/app/alert/slot-default.vue (1)
55-58: LGTM! Improved spacing for alert operations.The new CSS rule for anchor elements within
.demo-alertenhances the visual layout of the alert operations. The 16px margin-right provides appropriate spacing between links.packages/theme/src/base/transition.less (1)
235-245: LGTM! Consider adding a comment and clarifying class usage.The new alert transition classes are well-integrated and consistent with the existing transition styles. Good job on maintaining consistency in naming and behavior.
Consider adding a comment above these new classes to explain their purpose, similar to the comment at the beginning of the file. For example:
// Alert component fade transition classesCan you confirm if the omission of a
.tiny-transition-alert-fade-enter-activeclass is intentional? Most other transitions in this file include both enter-active and leave-active classes. If it's intentional, it might be worth adding a comment to explain why.examples/sites/demos/pc/app/alert/webdoc/alert.js (2)
54-57: Improved clarity in title customization descriptionThe updates to the 'title' demo description provide clearer and more detailed information about title customization:
- It now specifies that the title is only displayed when
sizeis set tolarge.- It explains how to customize the title using either the
titleprop or slot.- It mentions the default behavior when no custom title is provided.
These changes will help users better understand and utilize the title customization feature.
94-97: Improved clarity for custom interaction areaThe updates to both Chinese and English descriptions for the 'slot-default' demo provide valuable clarifications:
- They specify that the custom interaction area is only displayed when
sizeis set tolarge.- They clearly state that the custom area appears below the
descriptionvalue.These changes will help users better understand how and when to use the custom interaction area.
examples/sites/demos/apis/alert.js (1)
Line range hint
251-264: Summary of changes in Alert API documentationThe changes in this file improve the documentation for the Alert component by providing more detailed descriptions for the
defaultanddescriptionslots. These updates align well with the PR's objective of refactoring and enhancing the component's documentation.Key improvements:
- More specific information about the
defaultslot's visibility conditions and positioning.- Clearer description of the
descriptionslot's purpose.To further enhance the documentation:
- Ensure consistency between Chinese and English descriptions.
- Consider using markdown formatting instead of HTML tags for inline code.
Overall, these changes contribute positively to the component's documentation quality.
packages/theme/src/alert/index.less (1)
19-126: Code refactoring enhances maintainability and theming capabilitiesThe refactored CSS variables and the introduction of the
.inject-Alert-vars()mixin improve the maintainability and theming support of the Alert component. The changes are well-structured and align with best practices.
| // 按区域划分场景: | ||
| // Large 影响内容区要显示的区域，还影响 margin, padding 和 alert-icon的大小 | ||
| // Normal尺寸时： alert-icon + content(description) + close-icon | ||
| // Large 尺寸时： alert-icon + content(title/ description /opration) + close-icon | ||
| // 所以思路： 先按Large编写各区域场景，之后用Normal去覆盖某些值。 | ||
|
|
||
| // 按type(theme)划分: success/error/warn/info/simple 【叠加状态】 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider translating comments to English for consistency
The comments from lines 13 to 20 are in Chinese. If the project's coding standards prefer English for comments, translating them would enhance readability and maintainability for all contributors.
packages/theme/src/alert/vars.less
Outdated
| --tv-Alert-padding-x: var(--tv-space-xl); | ||
| // 警告的水平内边距 | ||
| --tv-Alert-padding-y: var(--tv-space-xl); |
There was a problem hiding this comment.
Correct the comment for vertical padding
On line 29, the variable --tv-Alert-padding-y represents the vertical padding, but the comment says "警告的水平内边距" (Alert's horizontal padding). Please update the comment to accurately reflect that this is the vertical padding.
Apply this diff to fix the comment:
-// 警告的水平内边距
+// 警告的垂直内边距📝 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.
| --tv-Alert-padding-x: var(--tv-space-xl); | |
| // 警告的水平内边距 | |
| --tv-Alert-padding-y: var(--tv-space-xl); | |
| --tv-Alert-padding-x: var(--tv-space-xl); | |
| // 警告的垂直内边距 | |
| --tv-Alert-padding-y: var(--tv-space-xl); |
packages/theme/src/alert/vars.less
Outdated
| --tv-Alert-margin-x: 0; | ||
| // 警告的水平外边距 | ||
| --tv-Alert-margin-y: 0; |
There was a problem hiding this comment.
Correct the comment for vertical margin
On line 33, the variable --tv-Alert-margin-y represents the vertical margin, but the comment says "警告的水平外边距" (Alert's horizontal margin). Please update the comment to accurately reflect that this is the vertical margin.
Apply this diff to fix the comment:
-// 警告的水平外边距
+// 警告的垂直外边距📝 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.
| --tv-Alert-margin-x: 0; | |
| // 警告的水平外边距 | |
| --tv-Alert-margin-y: 0; | |
| --tv-Alert-margin-x: 0; | |
| // 警告的水平外边距 | |
| --tv-Alert-margin-y: 0; | |
| // 警告的垂直外边距 |
packages/theme/src/alert/vars.less
Outdated
| --tv-Alert-margin-x-normal: 0; | ||
| // 警告的Normal时,水平外边距 | ||
| --tv-Alert-margin-y-normal: var(--tv-space-xl); |
There was a problem hiding this comment.
Correct the comment for vertical margin in Normal size
On line 87, the variable --tv-Alert-margin-y-normal represents the vertical margin for the Normal size, but the comment says "警告的Normal时,水平外边距" (Alert's Normal size, horizontal margin). Please update the comment to accurately reflect that this is the vertical margin.
Apply this diff to fix the comment:
-// 警告的Normal时,水平外边距
+// 警告的Normal时,垂直外边距📝 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.
| --tv-Alert-margin-x-normal: 0; | |
| // 警告的Normal时,水平外边距 | |
| --tv-Alert-margin-y-normal: var(--tv-space-xl); | |
| --tv-Alert-margin-x-normal: 0; | |
| // 警告的Normal时,垂直外边距 | |
| --tv-Alert-margin-y-normal: var(--tv-space-xl); |
packages/theme/src/alert/vars.less
Outdated
| --tv-Alert-padding-x-normal: var(--tv-space-xl); | ||
| // 警告的Normal时，水平内边距 | ||
| --tv-Alert-padding-y-normal: 9px; |
There was a problem hiding this comment.
Correct the comment for vertical padding in Normal size
On line 83, the variable --tv-Alert-padding-y-normal represents the vertical padding for the Normal size, but the comment says "警告的Normal时，水平内边距" (Alert's Normal size, horizontal padding). Please update the comment to accurately reflect that this is the vertical padding.
Apply this diff to fix the comment:
-// 警告的Normal时，水平内边距
+// 警告的Normal时，垂直内边距📝 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.
| --tv-Alert-padding-x-normal: var(--tv-space-xl); | |
| // 警告的Normal时，水平内边距 | |
| --tv-Alert-padding-y-normal: 9px; | |
| --tv-Alert-padding-x-normal: var(--tv-space-xl); | |
| // 警告的Normal时，垂直内边距 | |
| --tv-Alert-padding-y-normal: 9px; |
|
|
||
| // 按type(theme)划分: success/error/warn/info/simple 【叠加状态】 | ||
|
|
||
| .inject-Alert-vars() { |
There was a problem hiding this comment.
💡 Codebase verification
Old mixin .component-css-vars-alert() still referenced in theme-mobile package
The old mixin is still used in the following files:
packages/theme-mobile/src/alert/vars.lesspackages/theme-mobile/src/alert/index.less
Please update these references to .inject-Alert-vars() to prevent styling issues.
🔗 Analysis chain
Ensure the new mixin .inject-Alert-vars() is properly referenced
The method .component-css-vars-alert() has been removed and replaced with .inject-Alert-vars(). Please verify that all Less files and components that previously used .component-css-vars-alert() are updated to reference .inject-Alert-vars() to prevent styling issues.
Run the following script to check for any remaining references to the old method:
And to confirm the use of the new mixin:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to the old mixin `.component-css-vars-alert()`
# Test: Search for the old mixin usage. Expect: No occurrences found.
rg --type less $'\.component-css-vars-alert\('
Length of output: 193
Script:
# Test: Search for the new mixin usage. Expect: References where the new mixin is applied.
rg --type less $'\.inject-Alert-vars\('
Length of output: 158
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/alert/webdoc/alert.js (2)
22-23: Excellent improvements to the 'Size Mode' demo documentation.The changes significantly enhance the clarity and informativeness of the demo:
- The updated name 'Size Mode' better reflects the demo's content.
- The expanded description provides valuable details about the differences between 'normal' and 'large' modes.
- The use of HTML elements improves the structure and readability.
These improvements will greatly aid users in understanding the component's size modes.
Consider adding a class attribute to the
<ul>elements for consistent styling with other documentation, if applicable.Also applies to: 26-43
116-116: Comprehensive improvements to 'Custom close button' demo documentation.The changes significantly enhance the clarity and informativeness of the demo:
- The updated Chinese name better reflects the demo's content.
- The expanded descriptions provide detailed information about:
- The
closableproperty and its default value- The
close-textattribute and its behavior- The
closeslot for full customization- The added tip block highlights the fade animation feature.
These improvements will greatly aid users in understanding and implementing custom close button functionality.
Consider adding an example or link to the fade animation demonstration to make it easier for users to visualize the effect.
Also applies to: 120-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- examples/sites/demos/pc/app/alert/webdoc/alert.js (6 hunks)
🧰 Additional context used
🔇 Additional comments (7)
examples/sites/demos/pc/app/alert/webdoc/alert.js (7)
13-13: Improved readability in basic usage description.The addition of a line break (
<br>) in the Chinese description enhances readability by visually separating two pieces of information.
54-55: Enhanced clarity in 'Custom Title' demo description.The changes improve the documentation by:
- Providing more detailed information about when and how the title is displayed.
- Clarifying the default behavior when a custom title is not set.
- Making the English description more concise while maintaining clarity.
These updates will help users better understand the title customization options.
Also applies to: 57-57
64-65: Improved accuracy in 'Center content' demo naming and description.The changes enhance the documentation by:
- Updating the demo name to 'Center content', which more accurately describes the feature.
- Simplifying the Chinese description while maintaining clarity.
These updates provide a more precise representation of the centering functionality.
Also applies to: 68-68
77-77: Improved accuracy and clarity in 'Customized alert icon' demo.The changes enhance the documentation by:
- Updating the English demo name to 'Customized alert icon', which is more general and accurate.
- Rewording the descriptions to provide clearer information about icon customization and default behavior.
These updates offer a more precise and informative explanation of the icon customization feature.
Also applies to: 81-83
94-97: Enhanced clarity in 'Customized Interaction' demo description.The updates improve the documentation by:
- Specifying that the custom interaction area is only available when the size is set to 'large'.
- Clarifying the location of the custom interaction area relative to the description value.
These changes provide users with a more accurate understanding of how to use and what to expect from the customized interaction feature.
108-109: Improved precision in 'Display icon' demo description.The updates enhance the documentation by:
- Providing a more precise explanation of the
show-iconproperty's function.- Clarifying that the property specifically controls the visibility of the left icon.
These changes offer users a clearer understanding of how to control the icon's visibility.
Line range hint
1-153: Overall excellent improvements to Alert component documentation.The changes in this file significantly enhance the quality of the documentation for the Alert component. Key improvements include:
- More accurate and descriptive demo names.
- Expanded and clarified descriptions for various features.
- Better explanation of component behaviors and options.
- Improved readability through the use of HTML formatting.
These updates will greatly benefit users in understanding and implementing the Alert component effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/alert/basic-usage.spec.ts (2)
10-15: LGTM! Consider clarifying the comment.The updated assertions correctly check the background colors for different alert types, which aligns with the refactoring of the Alert component's CSS variables. This change improves the test coverage for the component's visual appearance.
Consider updating the comment on line 10 to be more specific:
- // 颜色背景正确 暂时无边框 + // 验证不同类型警告框的背景颜色 注意：当前无边框样式This change would provide more context about the different alert types being tested and clarify that the lack of border is intentional in the current design.
Line range hint
1-38: Test updated to reflect CSS refactoring changesThe modifications to this test file accurately reflect the CSS refactoring changes made to the Alert component. By shifting the focus from border colors to background colors, the test now provides better coverage for the updated visual design of the alert boxes.
Consider adding tests for other CSS properties that may have been affected by the refactoring, such as text colors, padding, or border-radius. This would ensure comprehensive coverage of the component's visual aspects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- examples/sites/demos/pc/app/alert/basic-usage.spec.ts (1 hunks)
- examples/sites/demos/pc/app/alert/size.spec.ts (0 hunks)
💤 Files with no reviewable changes (1)
- examples/sites/demos/pc/app/alert/size.spec.ts
🧰 Additional context used
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
alertcomponent, improving clarity on slot usage.Bug Fixes
Style