feat: add opened and closed events for dialog #4774
Conversation
|
WalkthroughThe pull request introduces enhancements to the 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: 3
🧹 Outside diff range and nitpick comments (8)
playground/src/views/examples/modal/base-demo.vue (2)
10-12: LGTM! Consider internationalizing the messages.The implementation of
onClosedandonOpenedhandlers aligns well with the PR objectives. However, the hardcoded Chinese messages should ideally be internationalized for better maintainability and global usage.Consider using an i18n system:
- message.info('onClosed：关闭动画结束'); + message.info(t('modal.animation.closed')); - message.info('onOpened：打开动画结束'); + message.info(t('modal.animation.opened'));Also applies to: 17-19
10-12: Add JSDoc comments to document the event timing.To improve code maintainability, consider adding JSDoc comments to clarify when these callbacks are triggered in relation to the animation lifecycle.
Add documentation like this:
+ /** + * Callback fired when the modal's close animation has completed + */ onClosed() { message.info('onClosed：关闭动画结束'); }, + /** + * Callback fired when the modal's open animation has completed + */ onOpened() { message.info('onOpened：打开动画结束'); },Also applies to: 17-19
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (2)
157-161: Consider documenting the callback execution order.While the
onOpenedcallback is well-documented, it would be helpful to clarify the order of execution in relation to other lifecycle callbacks (e.g.,onOpenChange). This helps developers understand the complete lifecycle flow.Consider adding a note in the documentation:
/** * 弹窗打开动画结束的回调 + * @remarks Triggered after onOpenChange(true) and animation completion * @returns */ onOpened?: () => void;
Line range hint
142-161: Consider grouping animation-related callbacks.For better maintainability and separation of concerns, consider extracting animation-related callbacks (
onOpened,onClosed) into a dedicated interface. This would make it easier to manage animation-specific functionality and potentially reuse it across other components.Example refactor:
interface AnimationCallbacks { /** Triggered when opening animation ends */ onOpened?: () => void; /** Triggered when closing animation ends */ onClosed?: () => void; } interface ModalApiOptions extends ModalState, AnimationCallbacks { // ... existing properties }packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (1)
68-68: Add documentation for animation states.The animation implementation uses data-[state] attributes and Tailwind classes for transitions. Consider adding comments to document the animation flow and available states.
Add documentation above the DialogContent component:
+ <!-- Animation States: + - data-[state=open]: Triggers entrance animations + - data-[state=closed]: Triggers exit animations + The animations include fade, zoom, and slide effects + --> <DialogContentAlso applies to: 71-74
docs/src/components/common-ui/vben-modal.md (1)
117-117: Consider enhancing event descriptions.While the current descriptions are accurate, consider making them more explicit about animation completion. Suggested improvements:
-| onClosed | 关闭动画播放完毕时触发 | `()=>void` | >5.4.3 | +| onClosed | 在对话框关闭且关闭动画完全结束后触发 | `()=>void` | >5.4.3 | -| onOpened | 打开动画播放完毕时触发 | `()=>void` | >5.4.3 | +| onOpened | 在对话框打开且打开动画完全结束后触发 | `()=>void` | >5.4.3 |Also applies to: 120-120
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (2)
191-191: Add null check for modalApi.The event handler is correctly implemented, but consider adding a null check to handle cases where modalApi might be undefined.
- @closed="() => modalApi?.onClosed()" + @closed="() => modalApi?.onClosed?.()"
196-196: Add null check for modalApi.The event handler is correctly implemented, but consider adding a null check to handle cases where modalApi might be undefined.
- @opened="() => modalApi?.onOpened()" + @opened="() => modalApi?.onOpened?.()"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
docs/src/components/common-ui/vben-modal.md(1 hunks)packages/@core/ui-kit/popup-ui/src/modal/__tests__/modal-api.test.ts(1 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts(4 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.ts(2 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.vue(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue(3 hunks)playground/src/views/examples/modal/base-demo.vue(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test (ubuntu-latest)
packages/@core/ui-kit/popup-ui/src/modal/__tests__/modal-api.test.ts
[failure] 125-125: packages/@core/ui-kit/popup-ui/src/modal/tests/modal-api.test.ts > modalApi > should call onOpened callback when provided
AssertionError: expected "spy" to be called at least once
❯ packages/@core/ui-kit/popup-ui/src/modal/tests/modal-api.test.ts:125:22
🔇 Additional comments (10)
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
142-146: LGTM! Well-documented callback for modal close animation.
The onClosed callback is properly typed and documented, following the existing pattern of lifecycle callbacks in the interface.
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)
30-32: LGTM: Event definitions are well-typed and follow conventions.
The new events 'opened' and 'closed' are properly typed and maintain consistency with the existing event structure.
49-55: 🛠️ Refactor suggestion
Verify animation handling specificity.
The current implementation might trigger for any animation, not just the dialog's open/close animations. Consider adding checks for the specific animation that completed.
Consider refining the implementation to be more specific:
-function onAnimationEnd() {
+function onAnimationEnd(event: AnimationEvent) {
+ // Only handle dialog animations
+ if (!event.target?.classList.contains('dialog-content')) {
+ return;
+ }
if (props.open) {
emits('opened');
} else {
emits('closed');
}
}packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (2)
9-14: LGTM! API interface changes are well-structured.
The new callback properties onClosed and onOpened are properly typed and maintain alphabetical ordering.
127-150: Verify event timing in integration tests.
The implementation correctly adds the onOpened and onClosed events as per PR objectives. To ensure proper timing of these events:
✅ Verification successful
Let me search for test files and any usage of these events to verify their implementation and testing coverage.
Event timing implementation and tests are properly in place
The implementation is well-tested with dedicated test cases for both onOpened and onClosed events in modal-api.test.ts. The events are:
- Properly typed in the interface
- Correctly initialized in the constructor
- Have proper state checks (
isOpen) before triggering - Covered by unit tests using mock functions to verify the callbacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new events are properly tested
# Test: Look for test cases covering animation completion
rg -A 5 "onOpened|onClosed" --type ts --type vue
Length of output: 78
Script:
#!/bin/bash
# Search for test files related to modal
fd "modal.*\.(test|spec)\.(ts|js|tsx|jsx)$"
# Search for onOpened/onClosed in all TypeScript files
rg -A 5 "onOpened|onClosed" --type ts
Length of output: 5151
docs/src/components/common-ui/vben-modal.md (2)
113-114: LGTM: Clear version column addition.
The addition of the version column in the events table improves clarity by explicitly showing when features were introduced.
117-117: LGTM: Well-documented new events.
The new events onClosed and onOpened are clearly documented with:
- Descriptive explanations of when they trigger
- Correct type signatures
- Version constraints (>5.4.3)
This aligns perfectly with the PR objectives of adding lifecycle events for dialog animations.
Also applies to: 120-120
packages/@core/ui-kit/popup-ui/src/modal/__tests__/modal-api.test.ts (2)
114-119: LGTM: Test case for onClosed follows established patterns.
The test case for onClosed callback follows the same patterns as other callback tests in the file and properly verifies the callback invocation.
114-126: Consider enhancing test coverage for the new callbacks.
The current tests only verify that callbacks are called when explicitly invoking onOpened/onClosed. Consider adding tests that verify:
- Callbacks are triggered after animation completion
- Callbacks receive correct parameters
- Error handling for invalid callbacks
Here's a suggested additional test case:
it('should handle invalid callbacks gracefully', () => {
const modalApiWithInvalidHook = new ModalApi({
onOpened: 'not a function' as any,
onClosed: null as any
});
expect(() => {
modalApiWithInvalidHook.open();
modalApiWithInvalidHook.onOpened();
}).not.toThrow();
expect(() => {
modalApiWithInvalidHook.close();
modalApiWithInvalidHook.onClosed();
}).not.toThrow();
});Let's verify the current test coverage:
🧰 Tools
🪛 GitHub Check: Test (ubuntu-latest)
[failure] 125-125: packages/@core/ui-kit/popup-ui/src/modal/tests/modal-api.test.ts > modalApi > should call onOpened callback when provided
AssertionError: expected "spy" to be called at least once
❯ packages/@core/ui-kit/popup-ui/src/modal/tests/modal-api.test.ts:125:22
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (1)
191-196: Verify event implementation completeness.
The new event handlers look good, but let's verify the complete implementation:
✅ Verification successful
Event implementation is properly verified and complete
The implementation is correctly structured and complete:
- DialogContent.vue properly emits 'opened' and 'closed' events through its onAnimationEnd handler
- ModalApi class includes onOpened and onClosed methods that are called at the appropriate times
- The interface and implementation are in sync with the event handlers in modal.vue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete implementation of opened and closed events
# Test 1: Check if DialogContent component emits these events
echo "Checking DialogContent for event emissions..."
rg -l "emit\(['\"](?:opened|closed)['\"]" --type vue
# Test 2: Check if modalApi interface includes these methods
echo "Checking modalApi interface..."
rg "interface.*ModalApi" -A 10 --type typescript
# Test 3: Check for existing usages that might need updates
echo "Checking existing usages..."
rg "modalApi\s*\.\s*on(?:Opened|Closed)" --type typescript --type vue
Length of output: 516
Script:
#!/bin/bash
# Let's try with more generic file extensions and patterns
# Check for event emissions in .vue files
echo "Checking for event emissions..."
find . -name "*.vue" -type f -exec grep -l "emit.*opened\|emit.*closed" {} \;
# Check for ModalApi interface
echo "Checking for ModalApi interface..."
find . -name "*.ts" -type f -exec grep -l "interface.*ModalApi" {} \;
# If we find the interface file, let's examine its content
echo "Examining ModalApi content if found..."
find . -name "*.ts" -type f -exec sh -c 'if grep -l "interface.*ModalApi" "$1" >/dev/null; then echo "=== $1 ==="; cat "$1"; fi' sh {} \;
# Check for existing usages
echo "Checking for modalApi usages..."
find . -name "*.vue" -name "*.ts" -type f -exec grep -l "modalApi\.on" {} \;
Length of output: 3273
Script:
#!/bin/bash
# Let's check the DialogContent.vue implementation to verify event emissions
echo "=== Examining DialogContent.vue content ==="
cat ./packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue
# Let's also check the modal-api.ts file since it was imported in modal.ts
echo "=== Examining modal-api.ts content ==="
cat ./packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts
Length of output: 6101
| onClosed, | ||
| onConfirm, | ||
| onOpenChange, | ||
| onOpened, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type annotations for callback parameters.
While the implementation is correct, consider explicitly typing the callbacks for better type safety and documentation:
constructor(options: ModalApiOptions = {}) {
const {
connectedComponent: _,
onBeforeClose,
onCancel,
- onClosed,
+ onClosed: (() => void) | undefined,
onConfirm,
onOpenChange,
- onOpened,
+ onOpened: (() => void) | undefined,
...storeState
} = options;Also applies to: 87-90
| /** | ||
| * 弹窗关闭动画播放完毕后的回调 | ||
| */ | ||
| onClosed() { | ||
| if (!this.state.isOpen) { | ||
| this.api.onClosed?.(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and type safety.
The implementation is good, but could be more robust with these improvements:
- onClosed() {
+ onClosed(): void {
if (!this.state.isOpen) {
- this.api.onClosed?.();
+ try {
+ this.api.onClosed?.();
+ } catch (error) {
+ console.error('Error in onClosed callback:', error);
+ }
}
}
- onOpened() {
+ onOpened(): void {
if (this.state.isOpen) {
- this.api.onOpened?.();
+ try {
+ this.api.onOpened?.();
+ } catch (error) {
+ console.error('Error in onOpened callback:', error);
+ }
}
}Also applies to: 143-150
| it('should call onOpened callback when provided', () => { | ||
| const onOpened = vi.fn(); | ||
| const modalApiWithHook = new ModalApi({ onOpened }); | ||
| modalApiWithHook.onOpened(); | ||
| expect(onOpened).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Test failure detected: onOpened callback verification.
The test is failing with the error "expected spy to be called at least once". This suggests that either:
- The
onOpenedmethod inModalApiis not properly implemented - The test needs to trigger the modal opening sequence before calling
onOpened
Consider modifying the test to include the complete modal lifecycle:
it('should call onOpened callback when provided', () => {
const onOpened = vi.fn();
const modalApiWithHook = new ModalApi({ onOpened });
+ modalApiWithHook.open();
modalApiWithHook.onOpened();
expect(onOpened).toHaveBeenCalled();
});📝 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.
| it('should call onOpened callback when provided', () => { | |
| const onOpened = vi.fn(); | |
| const modalApiWithHook = new ModalApi({ onOpened }); | |
| modalApiWithHook.onOpened(); | |
| expect(onOpened).toHaveBeenCalled(); | |
| }); | |
| it('should call onOpened callback when provided', () => { | |
| const onOpened = vi.fn(); | |
| const modalApiWithHook = new ModalApi({ onOpened }); | |
| modalApiWithHook.open(); | |
| modalApiWithHook.onOpened(); | |
| expect(onOpened).toHaveBeenCalled(); | |
| }); |
🧰 Tools
🪛 GitHub Check: Test (ubuntu-latest)
[failure] 125-125: packages/@core/ui-kit/popup-ui/src/modal/tests/modal-api.test.ts > modalApi > should call onOpened callback when provided
AssertionError: expected "spy" to be called at least once
❯ packages/@core/ui-kit/popup-ui/src/modal/tests/modal-api.test.ts:125:22
Description
为Dialog添加onOpened和onClosed事件。
onOpened: Dialog已打开并且动画播放完毕
onClosed: Dialog已关闭并且动画播放完毕
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Vben Modaldocumentation with new event handlers:onClosedandonOpened.DialogContentcomponent.Bug Fixes
Tests
onClosedandonOpenedcallbacks in the modal API.Documentation