refactor(notify): [notify] refactor notify theme #2311
Conversation
WalkthroughThe changes in this pull request involve significant updates to the notification component across various files. Key modifications include updates to API definitions, removal of the Changes
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (9)
examples/sites/demos/pc/app/notify/showClose.vue (1)
Line range hint
16-23: Consider the implications of removing thedurationproperty.The removal of the
durationproperty from theNotifyconfiguration might lead to unexpected behavior. Without a specified duration, the notification may stay visible indefinitely, which could be confusing for users who expect notifications to disappear after a certain time.Consider one of the following options:
- Re-add the
durationproperty with an appropriate value.- If the intention is to keep the notification visible until manually closed, ensure this behavior is clearly documented and consistent with the overall UX design of the application.
On a positive note, the use of JSX for the
titleproperty is a good practice, allowing for more flexible and dynamic title rendering.examples/sites/demos/pc/app/notify/position-composition-api.vue (1)
12-21: Approve script changes with a suggestion for improvement.The refactoring to use a single
handleClickfunction with the Composition API is a good improvement. It simplifies the code and makes it more flexible.Consider adding type checking for the
posparameter to ensure only valid positions are passed:function handleClick(pos: 'top-left' | 'top-right' | 'bottom-left' | 'bottom-right') { Notify({ title: '通知消息的标题', message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', position: pos }) }This change would provide better type safety and catch potential errors at compile-time.
examples/sites/demos/pc/app/notify/basic-usage.vue (3)
19-19: Minor text adjustment inhandleClickmethod.The
titleproperty of the Notify call has been slightly modified. Ensure this change is intentional and consistent with the overall messaging strategy.
26-36: LGTM: NewhandleClickJxsmethod demonstrates JSX usage.The new method effectively showcases the use of JSX syntax in Notify calls. This addition provides a clear example of how to use more complex content in notifications.
Consider adding a brief comment explaining the benefits of using JSX in this context, to help other developers understand the rationale behind this approach.
Line range hint
1-43: Summary: Effective demonstration of Notify component with JSX support.This update successfully introduces a new example of using JSX syntax with the Notify component, alongside the existing string literal approach. The changes are well-integrated and provide developers with a clear comparison between the two methods.
To further improve the educational value of this demo:
- Consider adding brief comments explaining when to use each approach (string literals vs. JSX).
- Ensure that the documentation is updated to reflect these new capabilities of the Notify component.
Overall, these changes enhance the demo's effectiveness in showcasing the Notify component's flexibility.
packages/vue/src/notify/index.ts (1)
Line range hint
1-180: Overall assessment of the notify implementation.The notify implementation appears to be well-structured and functional. The addition of new positioning options enhances its flexibility without introducing breaking changes. However, to ensure a comprehensive update:
- Review related CSS files to confirm proper styling for the new positions.
- Update any relevant documentation or examples to showcase the new positioning options.
- Consider adding or updating unit tests to cover the new functionality.
- Verify that the
debouncefunction and position calculations (lines 37-180) work correctly with the new positions.Consider extracting the position-related logic into a separate module to improve maintainability as the number of positioning options grows.
examples/sites/demos/pc/app/notify/webdoc/notify.js (1)
54-57: Great addition of default duration details!The updated descriptions provide valuable information about the default delay times for different message types. This clarification is crucial for developers to understand the behavior of notifications.
Consider adding a brief example of how to set a custom duration for even more clarity. For instance:
'zh-CN': '通过 <code>duration</code> 属性设置自动关闭的延迟时间。默认情况下，<code>success</code> 和 <code>info</code> 延时5秒，<code>warning</code> 和 <code>error</code> 延时10秒自动关闭。例如：<code>Notify({ type: "success", message: "操作成功", duration: 3000 })</code> 将在3秒后关闭。', 'en-US': 'Use the <code>duration</code> property to set the automatic shutdown delay. By default, <code>success</code> and <code>info</code> notifications close after 5 seconds, while <code>warning</code> and <code>error</code> close after 10 seconds. For example: <code>Notify({ type: "success", message: "Operation successful", duration: 3000 })</code> will close after 3 seconds.'packages/theme/src/notify/vars.less (1)
13-69: Translate comments to English for broader accessibilityThe comments in the code are written in Chinese. If the project aims for international collaboration, consider translating the comments into English to enhance readability and maintainability for all team members.
packages/theme/src/notify/index.less (1)
62-79: Check nesting and specificity of nested classesIn the message zone styles (lines 62-79), nested classes like
.@{notify-prefix-cls}__title-wrapperand.@{notify-prefix-cls}__content-wrapperare defined. Ensure that the nesting and specificity correctly apply the styles as intended, without unintended side effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
- examples/sites/demos/apis/notify.js (3 hunks)
- examples/sites/demos/pc/app/notify/basic-usage-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/notify/basic-usage.vue (2 hunks)
- examples/sites/demos/pc/app/notify/message-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/notify/message.spec.ts (0 hunks)
- examples/sites/demos/pc/app/notify/message.vue (0 hunks)
- examples/sites/demos/pc/app/notify/notify-events-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/notify/notify-events.vue (0 hunks)
- examples/sites/demos/pc/app/notify/position-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/notify/position.vue (1 hunks)
- examples/sites/demos/pc/app/notify/showClose-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/notify/showClose.vue (1 hunks)
- examples/sites/demos/pc/app/notify/showIcon-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/notify/showIcon.vue (1 hunks)
- examples/sites/demos/pc/app/notify/title-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/notify/title.spec.ts (0 hunks)
- examples/sites/demos/pc/app/notify/title.vue (0 hunks)
- examples/sites/demos/pc/app/notify/type-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/notify/type.vue (0 hunks)
- examples/sites/demos/pc/app/notify/verticalOffset-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/notify/verticalOffset.vue (0 hunks)
- examples/sites/demos/pc/app/notify/webdoc/notify.js (2 hunks)
- packages/theme/src/notify/index.less (1 hunks)
- packages/theme/src/notify/vars.less (1 hunks)
- packages/vue/src/notify/index.ts (2 hunks)
💤 Files with no reviewable changes (12)
- examples/sites/demos/pc/app/notify/message-composition-api.vue
- examples/sites/demos/pc/app/notify/message.spec.ts
- examples/sites/demos/pc/app/notify/message.vue
- examples/sites/demos/pc/app/notify/notify-events-composition-api.vue
- examples/sites/demos/pc/app/notify/notify-events.vue
- examples/sites/demos/pc/app/notify/title-composition-api.vue
- examples/sites/demos/pc/app/notify/title.spec.ts
- examples/sites/demos/pc/app/notify/title.vue
- examples/sites/demos/pc/app/notify/type-composition-api.vue
- examples/sites/demos/pc/app/notify/type.vue
- examples/sites/demos/pc/app/notify/verticalOffset-composition-api.vue
- examples/sites/demos/pc/app/notify/verticalOffset.vue
🧰 Additional context used
🔇 Additional comments (19)
examples/sites/demos/pc/app/notify/showClose-composition-api.vue (1)
13-17: Consider the impact of removing thedurationproperty.The
durationproperty has been removed from theNotifyfunction call. This change might affect the notification's display time, potentially making it persist indefinitely or use a default duration. Please ensure this behavior is intentional and consistent with the new design of the notify theme.The changes in the
baseClickfunction are consistent with the PR objectives of refactoring the notify theme. The use of JSX for the title and the retention of thepositionproperty align with the described updates.examples/sites/demos/pc/app/notify/showIcon.vue (2)
Line range hint
7-15: LGTM! Imports and component registration are correct.The imports from '@opentiny/vue' and the component registration are properly set up. The use of JSX syntax is also appropriate for this component.
Line range hint
1-5: LGTM! Consider clarifying thereset-timeprop.The template structure is appropriate for demonstrating the notification feature. However, the purpose of the
:reset-time="0"prop on the button is not immediately clear.Could you provide more information about the
reset-timeprop and its significance in this context? If it's not essential for this example, consider removing it to keep the demo focused on the notification feature.✅ Verification successful
reset-time属性在tiny-button组件中被广泛使用，且使用方式一致。因此，此处的使用是正确且符合预期的。🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of reset-time prop in button components rg --type vue 'tiny-button.*reset-time'Length of output: 69
Script:
#!/bin/bash # Search for other usages of reset-time prop in tiny-button components within .vue files fd --extension vue --exec rg 'tiny-button.*reset-time'Length of output: 19733
examples/sites/demos/pc/app/notify/position-composition-api.vue (2)
3-8: Approve template changes with a minor query.The template changes look good. The use of a single
handleClickmethod with position arguments improves code reusability and maintainability.Could you please clarify the purpose of the
:reset-time="0"prop on thetiny-buttoncomponents? It's not immediately clear what this prop does or why it's set to 0.
24-28: Approve style changes.The addition of scoped styles with a minimum width for the buttons is a good improvement. It enhances the visual consistency of the UI elements.
examples/sites/demos/pc/app/notify/basic-usage-composition-api.vue (3)
4-4: LGTM: New button added for JSX demonstration.The addition of a new button to demonstrate JSX functionality in notifications is a good enhancement. It provides users with a clear way to see the difference between standard and JSX-based notifications.
Line range hint
1-33: Overall assessment: Good refactoring with minor clarification needed.The changes in this file successfully introduce JSX support for notifications while maintaining the existing functionality. The code is well-structured and consistent. However, there's a minor discrepancy regarding the
durationproperty that needs clarification. Once addressed, these changes will significantly enhance the flexibility and capabilities of the notification system.
14-14: Consider implications of static title.The change from a render function to a static string for the
titleproperty simplifies the code. However, it removes the ability to use dynamic content in the title for this specific function. Ensure this aligns with the intended usage of this demo.To verify if dynamic titles are still needed elsewhere, run:
examples/sites/demos/pc/app/notify/position.vue (2)
31-35: LGTM! Good addition of scoped styles.The addition of scoped styles for
.tiny-buttonwith a minimum width of 120px is a good improvement. It will help maintain consistent button sizes across different text lengths, enhancing the overall appearance of the component.
3-8: LGTM! Consider clarifying the:reset-timeprop usage.The template changes look good. The use of a single
handleClickmethod with position arguments simplifies the code and improves maintainability. The buttons now cover all four corner positions, which is a nice improvement.Could you please clarify the purpose of the
:reset-time="0"prop on the buttons? It's not immediately clear what this prop does or why it's set to 0.✅ Verification successful
Verified the
:reset-timeprop usage across the codebase. It is consistently set to0with no issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of reset-time prop in the codebase # Expected result: Find other instances or documentation of reset-time prop rg --type vue 'reset-time' -C 2Length of output: 59
Script:
#!/bin/bash # Description: Check for other usages of reset-time prop in the codebase # Expected result: Find other instances or documentation of reset-time prop rg 'reset-time' --glob '*.vue' -C 2Length of output: 79989
examples/sites/demos/pc/app/notify/basic-usage.vue (1)
4-4: LGTM: New button added for JSX demonstration.The new button is consistent with the existing UI and clearly indicates its purpose to demonstrate JSX functionality.
examples/sites/demos/apis/notify.js (4)
64-64: LGTM: Updated demo reference formessageproperty.The change from 'message' to 'basic-usage' for the
pcDemovalue appears to be a refactoring of the demo structure. This update doesn't affect the functionality of the component and likely aligns with other demo references.
120-120: LGTM: Updated demo reference fortitleproperty.The change from 'title' to 'basic-usage' for the
pcDemovalue is consistent with the earlier change to themessageproperty. This update appears to be part of a broader refactoring of the demo structure and doesn't affect the component's functionality.
68-68: LGTM: Enhanced positioning options for notifications.The addition of 'top-left' and 'bottom-left' to the
positionproperty type increases the flexibility of the notify component. This is a positive, non-breaking change.To ensure these new positions are properly implemented, please run the following script:
#!/bin/bash # Search for the implementation of the position logic in the notify component rg --type vue -g '!*test*' -g '!*spec*' 'notify.*position.*top-left|notify.*position.*bottom-left'
44-50:⚠️ Potential issueClarify the default behavior for the
durationproperty.The
defaultValueis set to an empty string, but the description specifies default durations for different notification types. This might lead to confusion. Consider the following suggestions:
- If the empty string is intentional (to indicate no default duration):
- Update the description to clarify that these durations are applied when no duration is specified.
- If there are indeed default durations:
- Set the
defaultValueto match the description (e.g., '5000' for success/info types).- Clarify in the description how the different default durations are determined.
To verify the actual implementation, please run the following script:
examples/sites/demos/pc/app/notify/webdoc/notify.js (2)
12-29: Excellent improvements to the basic usage documentation!The expanded descriptions for both Chinese and English provide more comprehensive explanations of the Notify function usage. The addition of information about JSX and h function support is particularly valuable for developers.
These changes significantly enhance the usability and clarity of the component documentation.
Line range hint
1-154: Approve the removal of 'title' and 'message' demo objectsThe removal of the separate 'title' and 'message' demo objects appears to be a good decision for consolidating information. Their functionality is now covered in the expanded 'basic-usage' description, which reduces redundancy and simplifies the overall structure of the demos.
To ensure no crucial information has been lost, please run the following script to check if all key points from the removed demos are now covered in the 'basic-usage' demo:
✅ Verification successful
Removal of 'title' and 'message' demo objects verified successfully
The removal of the separate 'title' and 'message' demo objects has been successfully verified. Their functionality is properly covered in the 'basic-usage' demo, ensuring no information was lost.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all key information from removed demos is present in the basic-usage demo # Test: Check for presence of 'title' and 'message' explanations in basic-usage grep -A 10 "demoId: 'basic-usage'" examples/sites/demos/pc/app/notify/webdoc/notify.js | grep -E "title|message" # Expected output: Should see explanations for both 'title' and 'message' propertiesLength of output: 219
packages/theme/src/notify/index.less (2)
19-19: Ensure the.inject-Notify-vars();mixin is correctly defined and importedAt line 19, the mixin
.inject-Notify-vars();is used. Please verify that this mixin is properly defined and imported. If it's part of the refactoring, ensure that it's implemented correctly and available wherever needed.
44-54: Verify the status icon color variables are definedIn the notification type styles (lines 44-54), the
fillproperty uses CSS variables like--tv-Notify-status-icon-color-success,--tv-Notify-status-icon-color-info, etc. Please ensure that these variables are defined invars.lessor the appropriate variables file.
| <script> | ||
| import { Notify, Button } from '@opentiny/vue' | ||
|
|
||
| export default { | ||
| components: { | ||
| TinyButton: Button | ||
| }, | ||
| methods: { | ||
| infoClick() { | ||
| handleClick(pos) { | ||
| Notify({ | ||
| type: 'info', | ||
| title: (h, params) => <h4>通知消息的标题</h4>, | ||
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文' | ||
| }) | ||
| }, | ||
| successClick() { | ||
| Notify({ | ||
| type: 'success', | ||
| title: '通知消息的标题', | ||
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | ||
| position: 'top-right' | ||
| position: pos | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Multiple instances of Notify still use the type property and dynamic JSX for title. Please review these usages to ensure consistency and prevent potential issues.
🔗 Analysis chain
LGTM! Please clarify changes to title and notification type.
The script changes look good overall. The consolidation of notification handling into a single handleClick method improves code organization and maintainability.
I have a couple of questions:
- The title is now a static string instead of JSX. Was this intentional? Does it reduce flexibility in any way?
- The
typeproperty has been removed from theNotifycall. How does this affect the appearance or behavior of the notifications?
To help verify the impact of these changes, could you run the following script?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of Notify with title and type properties
# Expected result: Find other instances of Notify to compare implementation
rg --type vue 'Notify\({' -A 5 -B 1
Length of output: 119
Script:
#!/bin/bash
# Description: Check for other usages of Notify with title and type properties
# Expected result: Find other instances of Notify to compare implementation
rg --type vue 'Notify\(\{' -A 5 -B 1
Length of output: 66
Script:
#!/bin/bash
# Description: Check for other usages of Notify with title and type properties
# Expected result: Find other instances of Notify to compare implementation
rg --glob '*.vue' 'Notify\(\{' -A 5 -B 1
Length of output: 95098
| @@ -32,7 +32,7 @@ const durationMap = { | |||
| error: 10000 | |||
| } | |||
|
|
|||
| const positionList = ['top-right', 'bottom-right'] | |||
| const positionList = ['top-right', 'bottom-right', 'top-left', 'bottom-left'] | |||
There was a problem hiding this comment.
💡 Codebase verification
CSS support for new notification positions confirmed.
The CSS files have been updated to include styles for 'top-left' and 'bottom-left' positions, ensuring the notification system supports the new placements.
However, the following areas require attention to complete the implementation:
-
Documentation:
- No updates found in documentation files to reflect the new positioning options. Ensure that the documentation is updated to guide users on using 'top-left' and 'bottom-left' positions.
-
Testing:
- Tests do not currently cover the new positions. Add test cases to verify that notifications can be positioned correctly using 'top-left' and 'bottom-left'.
🔗 Analysis chain
Approve the addition of new notification positions with suggestions.
The expansion of the positionList array to include 'top-left' and 'bottom-left' positions is a good improvement, offering more flexibility in notification placement. This change enhances the user experience without breaking existing functionality.
However, to ensure full implementation:
- Update the documentation to reflect these new positioning options.
- Verify that the necessary CSS changes have been made to support the new 'top-left' and 'bottom-left' positions.
- Consider adding new tests to cover these additional positioning options.
To verify the complete implementation, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSS changes and documentation updates for new notification positions
# Test 1: Check for CSS changes related to new positions
echo "Checking for CSS changes:"
rg --type css -e 'top-left|bottom-left' packages/vue/src/notify
# Test 2: Check for documentation updates
echo "Checking for documentation updates:"
rg --type md -e 'top-left|bottom-left' docs/
# Test 3: Check for test updates
echo "Checking for test updates:"
rg --type js -e 'top-left|bottom-left' tests/
Length of output: 458
Script:
#!/bin/bash
# Description: Comprehensive verification for new notification positions
# Test 1: Search for CSS changes related to new positions across the entire repository
echo "Searching for CSS changes related to 'top-left' or 'bottom-left':"
rg --type css 'top-left|bottom-left'
# Test 2: Search for documentation updates related to new positions across the entire repository
echo "Searching for documentation updates related to 'top-left' or 'bottom-left':"
rg --type md 'top-left|bottom-left'
# Test 3: Search for test updates related to new positions across the entire repository
echo "Searching for test updates related to 'top-left' or 'bottom-left':"
rg --type js 'top-left|bottom-left'
# Test 4: Verify if 'positionList' is used in other components
echo "Searching for usages of 'positionList' in the codebase:"
rg 'positionList'
Length of output: 3869
Script:
#!/bin/bash
# Description: Comprehensive verification for new notification positions across the entire repository
# Test 1: Search for CSS changes related to 'top-left' or 'bottom-left' in all CSS files
echo "Searching for CSS changes related to 'top-left' or 'bottom-left':"
rg --type css 'top-left|bottom-left'
# Test 2: Search for documentation updates related to 'top-left' or 'bottom-left' in all Markdown files
echo "Searching for documentation updates related to 'top-left' or 'bottom-left':"
rg --type md 'top-left|bottom-left'
# Test 3: Search for test updates related to 'top-left' or 'bottom-left' in all JavaScript and TypeScript test files
echo "Searching for test updates related to 'top-left' or 'bottom-left':"
rg --type js --type ts 'top-left|bottom-left'
# Test 4: Search for any remaining references to 'top-left' or 'bottom-left' in the entire codebase
echo "Searching for any other references to 'top-left' or 'bottom-left' in the codebase:"
rg 'top-left|bottom-left'
# Test 5: Verify if 'positionList' is used in other components for consistency
echo "Searching for usages of 'positionList' in the codebase:"
rg 'positionList'
Length of output: 10653
| // 通知的行高 | ||
| --tv-Notify-max-width: 340px; |
There was a problem hiding this comment.
Correct the mismatched comment on line 15~
The comment on line 15~ says "通知的行高" (notification line height), but the variable on line 16~ is --tv-Notify-max-width: 340px;, which sets the maximum width of the notification, not the line height. Please update the comment to accurately reflect the variable's purpose.
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-Notify-max-width: 340px; | |
| // 通知的最大宽度 | |
| --tv-Notify-max-width: 340px; |
| .inject-Notify-vars() { | ||
| //---------------------------------------------------------------- 全局场景 | ||
| // 通知的行高 | ||
| --tv-Notify-max-width: 340px; | ||
| // 通知的行高 | ||
| --tv-Notify-line-height: var(--tv-line-height-number); | ||
| // 通知的背景色 | ||
| --tv-Notify-bg-color: var(--tv-color-bg-secondary); | ||
| // 通知的圆角 | ||
| --tv-Notify-border-radius: var(--tv-border-radius-lg); | ||
| // 通知的外阴影 | ||
| --tv-Notify-box-shadow: var(--tv-shadow-2-down); | ||
| // 通知的水平内边距 | ||
| --tv-Notify-padding-x: var(--tv-space-xl); | ||
| // 通知的垂直内边距 | ||
| --tv-Notify-padding-y: var(--tv-space-xl); | ||
| // ----------------------------------------------------------------状态图标 | ||
| // 状态图标的大小 | ||
| --tv-Notify-status-icon-font-size: 24px; | ||
| // 消息状态图标的右外边距 | ||
| --tv-Notify-status-icon-margin-right: var(--tv-space-md); | ||
| // 成功状态时，图标的颜色 | ||
| --tv-Notify-status-icon-color-success: var(--tv-color-success-icon); | ||
| // 错误状态时，图标的颜色 | ||
| --tv-Notify-status-icon-color-error: var(--tv-color-error-icon); | ||
| // 警告状态时，图标的颜色 | ||
| --tv-Notify-status-icon-color-warning: var(--tv-color-warn-icon); | ||
| // 信息状态时，图标的颜色 | ||
| --tv-Notify-status-icon-color-info: var(--tv-color-info-icon); | ||
|
|
||
| // ----------------------------------------------------------------消息区 title+message | ||
| // 通知消息区的水平外边距 | ||
| --tv-Notify-msg-margin-x: var(--tv-space-md); | ||
|
|
||
| // 通知标题的字体颜色 | ||
| --tv-Notify-title-text-color: var(--tv-color-text); | ||
| // 通知标题的字体大小 | ||
| --tv-Notify-title-font-size: var(--tv-font-size-lg); | ||
| // 通知标题的字重 | ||
| --tv-Notify-title-font-weight: var(--tv-font-weight-bold); | ||
| // 通知标题的下边距外边距 | ||
| --tv-Notify-title-margin-bottom: var(--tv-space-md); | ||
|
|
||
| // 通知内容的字体颜色 | ||
| --tv-Notify-msg-text-color: var(--tv-color-text-secondary); | ||
| // 通知内容的字体大小 | ||
| --tv-Notify-msg-font-size: var(--tv-font-size-md); | ||
| // 通知内容的最大高度 | ||
| --tv-Notify-msg-max-height: 96px; | ||
|
|
||
| // ----------------------------------------------------------------关闭图标 | ||
| // 关闭图标的大小 | ||
| --tv-Notify-close-icon-font-size: 20px; | ||
| // 关闭图标的颜色 | ||
| --tv-Notify-close-icon-color: var(--tv-color-icon); | ||
| // 关闭图标的悬浮 | ||
| --tv-Notify-close-icon-color-hover: var(--tv-color-icon-hover); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider standardizing CSS variable naming conventions
The custom property names use a mix of uppercase and lowercase letters, such as --tv-Notify-max-width and --tv-Notify-bg-color. For consistency and to follow CSS best practices, consider using all lowercase letters with hyphens for custom property names.
Apply this diff to update the variable names:
- --tv-Notify-max-width: 340px;
- --tv-Notify-line-height: var(--tv-line-height-number);
- --tv-Notify-bg-color: var(--tv-color-bg-secondary);
+ --tv-notify-max-width: 340px;
+ --tv-notify-line-height: var(--tv-line-height-number);
+ --tv-notify-bg-color: var(--tv-color-bg-secondary);Note: Update all occurrences of --tv-Notify- to --tv-notify- for consistency.
Committable suggestion was skipped due to low confidence.
| &.top-left { | ||
| left: var(--ti-notify-position-top-left); | ||
| top: 25px; | ||
| /** 状态图标 */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider translating comments to English for consistency
The comments on lines 37, 57, 83, and 96 are written in Chinese. For better accessibility and maintainability across the team and international contributors, it's recommended to use English for code comments.
Also applies to: 57-57, 83-83, 96-96
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
重构主题
补充文档的位置/延时的描述
移除2个demo
Summary by CodeRabbit
Release Notes
New Features
notifycomponent API with additional positioning options (top-left, bottom-left).Improvements
Bug Fixes
durationproperty from various notification instances to simplify configuration.Deprecations