fix(notify): [notify] add manual close demo for notify #2393
Conversation
WalkthroughThis pull request introduces a new Vue component for manual notification closure and modifies existing demo descriptions for clarity. The Changes
Possibly related PRs
Suggested labels
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (5)
examples/sites/demos/pc/app/notify/manual-close.spec.ts (1)
1-18: Consider adding more test scenarios.The current test only covers the basic flow. Consider adding tests for:
- Individual notification closure
- Error cases (e.g., closing non-existent notifications)
- Different notification types
Would you like me to help generate additional test scenarios?
examples/sites/demos/pc/app/notify/manual-close-composition-api.vue (1)
1-7: Consider internationalizing button labels and adding proper styling.The template could benefit from the following improvements:
- Button labels should use i18n for better internationalization support
- The 'content' class lacks styling definitions which might affect layout consistency
Consider applying these changes:
<template> - <div class="content"> + <div class="manual-close-demo"> - <tiny-button @click="manualCloseClick" :reset-time="0">通过 instance 关闭</tiny-button> - <tiny-button @click="closebyIdClick" :reset-time="0">通过 id 关闭</tiny-button> - <tiny-button @click="closeAllClick" :reset-time="0">批量全部关闭</tiny-button> + <tiny-button @click="manualCloseClick" :reset-time="0">{{ t('closeViaInstance') }}</tiny-button> + <tiny-button @click="closebyIdClick" :reset-time="0">{{ t('closeViaId') }}</tiny-button> + <tiny-button @click="closeAllClick" :reset-time="0">{{ t('closeAll') }}</tiny-button> </div> </template>Add corresponding styles:
<style scoped> .manual-close-demo { display: flex; gap: 1rem; } </style>examples/sites/demos/pc/app/notify/manual-close.vue (2)
1-6: Consider adding accessibility attributes and i18n support.The buttons could benefit from:
- ARIA labels for screen readers
- Internationalization support for button text
- Documentation for the
:reset-time="0"prop usageConsider updating the template:
<div class="content"> - <tiny-button @click="manualCloseClick" :reset-time="0">通过 instance 关闭</tiny-button> - <tiny-button @click="closebyIdClick" :reset-time="0">通过 id 关闭</tiny-button> - <tiny-button @click="closeAllClick" :reset-time="0">批量全部关闭</tiny-button> + <tiny-button + @click="manualCloseClick" + :reset-time="0" + aria-label="Close notification via instance"> + {{ t('closeViaInstance') }} + </tiny-button> + <tiny-button + @click="closebyIdClick" + :reset-time="0" + aria-label="Close notification by ID"> + {{ t('closeViaId') }} + </tiny-button> + <tiny-button + @click="closeAllClick" + :reset-time="0" + aria-label="Close all notifications"> + {{ t('closeAll') }} + </tiny-button>
1-60: Add JSDoc documentation for the component and its methods.The component would benefit from proper documentation explaining its purpose and usage.
Add documentation at the component level:
+/** + * @component ManualClose + * @description Demonstrates different methods of closing notifications: + * - Closing via instance + * - Closing via ID + * - Closing all notifications + */ export default { // ... existing code }examples/sites/demos/pc/app/notify/webdoc/notify.js (1)
161-162: Improve method signature formatting for better readability.The method signatures could be formatted more consistently and clearly. Consider using proper TypeScript-style method signatures.
- 1、<code>Notify.close :(id, beforeClose)=>void </code>, 关闭指定的通知。其中<code>id</code>可通过<code>instance</code>对象获取。 <br> - 2、<code>Notify.closeAll :()=>void </code>, 关闭所有通知。<br> + 1、<code>Notify.close(id: string, beforeClose?: () => void): void</code>, 关闭指定的通知。其中<code>id</code>可通过<code>instance</code>对象获取。 <br> + 2、<code>Notify.closeAll(): void</code>, 关闭所有通知。<br>And similarly for the English translation:
- 1, <code>Notify.close :(id, beforeClose)=>void </code>, turn off the specified notification. The <code>id</code> can be obtained from the <code>instance</code> object. <br> - 2. <code>Notify.closeAll :()=>void </code> to close all notifications. <br> + 1. <code>Notify.close(id: string, beforeClose?: () => void): void</code>, turn off the specified notification. The <code>id</code> can be obtained from the <code>instance</code> object. <br> + 2. <code>Notify.closeAll(): void</code>, to close all notifications. <br>Also applies to: 168-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/sites/demos/pc/app/alert/webdoc/alert.js (1 hunks)
- examples/sites/demos/pc/app/notify/manual-close-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/notify/manual-close.spec.ts (1 hunks)
- examples/sites/demos/pc/app/notify/manual-close.vue (1 hunks)
- examples/sites/demos/pc/app/notify/webdoc/notify.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/sites/demos/pc/app/alert/webdoc/alert.js
🔇 Additional comments (4)
examples/sites/demos/pc/app/notify/manual-close.spec.ts (1)
1-2: LGTM: Imports are correct and minimal.The test imports exactly what's needed from Playwright.
examples/sites/demos/pc/app/notify/manual-close-composition-api.vue (2)
9-10: LGTM!The imports are clean and follow Vue best practices.
1-53: Verify notification system integration and usage patterns.Let's verify the correct usage of the Notify API and check for similar patterns in the codebase.
✅ Verification successful
Based on the search results, I can verify that the Notify API usage in the code is correct and consistent with other implementations in the codebase. The code follows the standard pattern for using Notify's close and closeAll methods, which are properly defined in the core implementation (
packages/vue/src/notify/index.ts).Notify API integration is correctly implemented.
- The code uses standard Notify API methods (close, closeAll) that are properly defined in the core implementation
- The notification instance management and cleanup approach matches other examples in the codebase
- The setTimeout cleanup pattern is commonly used across other notification examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Notify API usage patterns and potential issues # Test 1: Check for other files using Notify.close or Notify.closeAll echo "Checking Notify API usage patterns..." rg -l "Notify\.(close|closeAll)" --type vue # Test 2: Check for potential memory leaks (missing cleanup) echo "Checking for potential memory leak patterns..." rg "setTimeout.*Notify" --type vue # Test 3: Check for hardcoded notification messages echo "Checking for hardcoded notification messages..." rg "Notify\({[^}]*message:\s*['\"]" --type vueLength of output: 603
Script:
#!/bin/bash # Let's try a different approach with proper regex patterns # Check for Notify API usage patterns echo "=== Checking Notify API usage patterns ===" rg "Notify\.(close|closeAll)" -t vue # Check for setTimeout with Notify echo -e "\n=== Checking setTimeout patterns ===" rg "setTimeout.*Notify" -t vue # Check notification message patterns echo -e "\n=== Checking notification message patterns ===" rg "Notify\([^)]+message:" -t vue # Check for similar notification component usage echo -e "\n=== Checking overall Notify usage ===" rg "import.*Notify.*from '@opentiny/vue'" -t vueLength of output: 660
Script:
#!/bin/bash # Let's try without file type filtering since 'vue' is not recognized # Check for Notify API usage patterns echo "=== Checking Notify API usage patterns ===" rg "Notify\.(close|closeAll)" # Check for setTimeout with Notify echo -e "\n=== Checking setTimeout patterns ===" rg "setTimeout.*Notify" # Check notification message patterns echo -e "\n=== Checking notification message patterns ===" rg "Notify\([^)]+message:" # Check for similar notification component usage echo -e "\n=== Checking overall Notify usage ===" rg "import.*Notify.*from '@opentiny/vue'" # Let's also check the directory structure to understand file organization echo -e "\n=== Checking directory structure ===" fd -e vueLength of output: 292563
examples/sites/demos/pc/app/notify/webdoc/notify.js (1)
150-173: LGTM! The demo integration is well-structured.The new manual-close demo is properly integrated into the existing demo structure, following the established patterns for demo configuration.
| test('手动关闭通知', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling.
While the error handler is good, it could be more informative for debugging.
Consider this enhancement:
- page.on('pageerror', (exception) => expect(exception).toBeNull())
+ page.on('pageerror', (exception) => {
+ console.error('Page error:', exception);
+ expect(exception).toBeNull();
+ })📝 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.
| test('手动关闭通知', async ({ page }) => { | |
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | |
| test('手动关闭通知', async ({ page }) => { | |
| page.on('pageerror', (exception) => { | |
| console.error('Page error:', exception); | |
| expect(exception).toBeNull(); | |
| }) |
| const demo = page.locator('.pc-demo') | ||
| const buttons = demo.locator('.tiny-button') | ||
| const notifys = page.locator('.tiny-notify') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding element existence checks.
The locators are correctly defined, but their existence should be verified before use.
Add existence checks:
const demo = page.locator('.pc-demo')
+ await expect(demo).toBeVisible()
const buttons = demo.locator('.tiny-button')
+ await expect(buttons).toHaveCount(3)
const notifys = page.locator('.tiny-notify')📝 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.
| const demo = page.locator('.pc-demo') | |
| const buttons = demo.locator('.tiny-button') | |
| const notifys = page.locator('.tiny-notify') | |
| const demo = page.locator('.pc-demo') | |
| await expect(demo).toBeVisible() | |
| const buttons = demo.locator('.tiny-button') | |
| await expect(buttons).toHaveCount(3) | |
| const notifys = page.locator('.tiny-notify') |
| await buttons.nth(0).click() | ||
| await buttons.nth(1).click() | ||
| await buttons.nth(2).click() | ||
|
|
There was a problem hiding this comment.
Add assertions between button clicks.
The test clicks multiple buttons in sequence without verifying the intermediate states.
Add intermediate assertions to ensure each click has the expected effect:
await buttons.nth(0).click()
+ await expect(notifys).toHaveCount(1)
await buttons.nth(1).click()
+ await expect(notifys).toHaveCount(3)
await buttons.nth(2).click()
+ await expect(notifys).toHaveCount(5)Committable suggestion was skipped due to low confidence.
| await expect(notifys).toHaveCount(5) | ||
| await page.waitForTimeout(4100) | ||
| await expect(notifys).toHaveCount(0) |
There was a problem hiding this comment.
Avoid fixed timeouts in tests.
Using waitForTimeout makes the test brittle and can lead to flaky tests.
Replace the fixed timeout with a more reliable wait condition:
await expect(notifys).toHaveCount(5)
- await page.waitForTimeout(4100)
+ await expect(notifys).toHaveCount(0, { timeout: 5000 })
- await expect(notifys).toHaveCount(0)📝 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.
| await expect(notifys).toHaveCount(5) | |
| await page.waitForTimeout(4100) | |
| await expect(notifys).toHaveCount(0) | |
| await expect(notifys).toHaveCount(5) | |
| await expect(notifys).toHaveCount(0, { timeout: 5000 }) |
| function closebyIdClick() { | ||
| const instance = Notify({ | ||
| type: 'info', | ||
| title: '通知', | ||
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | ||
| position: 'top-right', | ||
| showClose: false | ||
| }) | ||
|
|
||
| setTimeout(() => { | ||
| Notify.close(instance.id, () => console.log(`即将关闭 id=${instance.id} 的通知`)) | ||
| }, 4000) | ||
| } |
There was a problem hiding this comment.
Add unique identifiers and improve error handling for ID-based closure.
The closebyIdClick method should:
- Use a more descriptive identifier for tracking
- Handle potential errors during closure
- Use consistent timing constants
Apply these improvements:
+const NOTIFICATION_DURATION = 4000
+
function closebyIdClick() {
const instance = Notify({
type: 'info',
- title: '通知',
- message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文',
+ title: t('notification'),
+ message: t('notificationMessage'),
position: 'top-right',
- showClose: false
+ showClose: false,
+ id: `manual-close-demo-${Date.now()}`
})
setTimeout(() => {
- Notify.close(instance.id, () => console.log(`即将关闭 id=${instance.id} 的通知`))
+ try {
+ Notify.close(instance.id, () => {
+ console.debug(`Closing notification ${instance.id}`)
+ })
+ } catch (error) {
+ console.error(`Failed to close notification ${instance.id}:`, error)
+ }
- }, 4000)
+ }, NOTIFICATION_DURATION)
}Committable suggestion was skipped due to low confidence.
| <script> | ||
| import { Button, Notify } from '@opentiny/vue' | ||
|
|
||
| export default { | ||
| components: { | ||
| TinyButton: Button | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding TypeScript support for better type safety.
The component could benefit from TypeScript's type checking and IDE support.
Consider updating the imports and component registration:
-<script>
+<script lang="ts">
import { Button, Notify } from '@opentiny/vue'
+import { defineComponent } from 'vue'
-export default {
+export default defineComponent({
components: {
TinyButton: Button
},
-}
+})📝 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.
| <script> | |
| import { Button, Notify } from '@opentiny/vue' | |
| export default { | |
| components: { | |
| TinyButton: Button | |
| }, | |
| <script lang="ts"> | |
| import { Button, Notify } from '@opentiny/vue' | |
| import { defineComponent } from 'vue' | |
| export default defineComponent({ | |
| components: { | |
| TinyButton: Button | |
| }, | |
| }) |
| closeAllClick() { | ||
| for (let i = 1; i <= 3; i++) { | ||
| Notify({ | ||
| type: 'info', | ||
| title: '通知' + i, | ||
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | ||
| position: 'top-right', | ||
| showClose: false | ||
| }) | ||
| } | ||
|
|
||
| setTimeout(() => { | ||
| Notify.closeAll() | ||
| }, 4000) | ||
| } |
There was a problem hiding this comment.
Improve closeAllClick implementation with proper cleanup.
The method creates multiple notifications without proper cleanup and error handling.
Consider updating:
closeAllClick() {
+ const instances = [];
+ try {
for (let i = 1; i <= 3; i++) {
- Notify({
- type: 'info',
- title: '通知' + i,
- message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文',
- position: 'top-right',
- showClose: false
- })
+ const instance = Notify({
+ ...DEFAULT_NOTIFY_CONFIG,
+ title: `通知${i}`
+ });
+ instances.push(instance);
}
- setTimeout(() => {
+ const timer = setTimeout(() => {
Notify.closeAll()
- }, 4000)
+ }, NOTIFICATION_DELAY);
+
+ this.$once('hook:beforeDestroy', () => {
+ clearTimeout(timer);
+ // Ensure cleanup of any remaining notifications
+ instances.forEach(instance => {
+ try {
+ instance.close();
+ } catch (error) {
+ console.error('Failed to close notification:', error);
+ }
+ });
+ });
+ } catch (error) {
+ console.error('Failed to create or close notifications:', error);
+ }
}Committable suggestion was skipped due to low confidence.
| closebyIdClick() { | ||
| const instance = Notify({ | ||
| type: 'info', | ||
| title: '通知', | ||
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | ||
| position: 'top-right', | ||
| showClose: false | ||
| }) | ||
|
|
||
| setTimeout(() => { | ||
| Notify.close(instance.id, () => console.log(`即将关闭 id=${instance.id} 的通知`)) | ||
| }, 4000) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup for closebyIdClick.
The method needs error handling and timeout cleanup.
Consider updating:
closebyIdClick() {
+ try {
- const instance = Notify({
- type: 'info',
- title: '通知',
- message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文',
- position: 'top-right',
- showClose: false
- })
+ const instance = Notify(DEFAULT_NOTIFY_CONFIG);
+ const timer = setTimeout(() => {
+ Notify.close(instance.id, () => {
+ console.log(`即将关闭 id=${instance.id} 的通知`);
+ });
+ }, NOTIFICATION_DELAY);
- setTimeout(() => {
- Notify.close(instance.id, () => console.log(`即将关闭 id=${instance.id} 的通知`))
- }, 4000)
+ this.$once('hook:beforeDestroy', () => {
+ clearTimeout(timer);
+ });
+ } catch (error) {
+ console.error('Failed to create or close notification:', error);
+ }
},Committable suggestion was skipped due to low confidence.
| manualCloseClick() { | ||
| const instance = Notify({ | ||
| type: 'info', | ||
| title: '通知', | ||
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | ||
| position: 'top-right', | ||
| showClose: false | ||
| }) | ||
|
|
||
| setTimeout(() => { | ||
| instance.close() | ||
| }, 4000) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common configuration and add error handling for manualCloseClick.
The notification configuration is duplicated across methods and there's no error handling.
Consider refactoring:
+const NOTIFICATION_DELAY = 4000;
+const DEFAULT_NOTIFY_CONFIG = {
+ type: 'info',
+ title: '通知',
+ message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文',
+ position: 'top-right',
+ showClose: false
+};
methods: {
manualCloseClick() {
- const instance = Notify({
- type: 'info',
- title: '通知',
- message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文',
- position: 'top-right',
- showClose: false
- })
+ try {
+ const instance = Notify(DEFAULT_NOTIFY_CONFIG);
+ const timer = setTimeout(() => {
+ instance.close();
+ }, NOTIFICATION_DELAY);
- setTimeout(() => {
- instance.close()
- }, 4000)
+ // Cleanup on component unmount
+ this.$once('hook:beforeDestroy', () => {
+ clearTimeout(timer);
+ });
+ } catch (error) {
+ console.error('Failed to create or close notification:', error);
+ }
},📝 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.
| manualCloseClick() { | |
| const instance = Notify({ | |
| type: 'info', | |
| title: '通知', | |
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | |
| position: 'top-right', | |
| showClose: false | |
| }) | |
| setTimeout(() => { | |
| instance.close() | |
| }, 4000) | |
| }, | |
| const NOTIFICATION_DELAY = 4000; | |
| const DEFAULT_NOTIFY_CONFIG = { | |
| type: 'info', | |
| title: '通知', | |
| message: '通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文，通知消息的正文', | |
| position: 'top-right', | |
| showClose: false | |
| }; | |
| manualCloseClick() { | |
| try { | |
| const instance = Notify(DEFAULT_NOTIFY_CONFIG); | |
| const timer = setTimeout(() => { | |
| instance.close(); | |
| }, NOTIFICATION_DELAY); | |
| // Cleanup on component unmount | |
| this.$once('hook:beforeDestroy', () => { | |
| clearTimeout(timer); | |
| }); | |
| } catch (error) { | |
| console.error('Failed to create or close notification:', error); | |
| } | |
| }, |
| 'en-US': 'Vertical Offset' | ||
| }, |
There was a problem hiding this comment.
Fix incorrect English translation for the demo name.
The English name "Vertical Offset" does not match the Chinese name "手动关闭通知" (Manual Close Notification) and does not reflect the demo's purpose of showing manual notification closure.
- 'en-US': 'Vertical Offset'
+ 'en-US': 'Manual Close Notification'📝 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.
| 'en-US': 'Vertical Offset' | |
| }, | |
| 'en-US': 'Manual Close Notification' | |
| }, |
shenjunjian
changed the title
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
Notifyfunctions.Bug Fixes
Tests