feat: add opened and closed events for dialog #4773
Conversation
|
WalkthroughThe pull request introduces enhancements to the Vben Modal component, adding two new event handlers, 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: 3
🧹 Outside diff range and nitpick comments (5)
playground/src/views/examples/modal/base-demo.vue (3)
10-12: LGTM: onClosed callback implementation.The implementation correctly demonstrates the usage of the new
onClosedevent that fires after the closing animation ends.Consider using internationalization for the message text instead of hardcoding the Chinese string:
- message.info('onClosed：关闭动画结束'); + message.info(t('modal.onClosed'));
17-19: LGTM: onOpened callback implementation.The implementation correctly demonstrates the usage of the new
onOpenedevent that fires after the opening animation ends.Consider using internationalization for the message text instead of hardcoding the Chinese string:
- message.info('onOpened：打开动画结束'); + message.info(t('modal.onOpened'));
Line range hint
1-27: Consider enhancing the demo with documentation.Since this is a demo file showcasing the new modal events, it would be helpful to:
- Add comments explaining the purpose of each callback
- Consider adding more practical examples of how these events can be used in real applications
- Use internationalization for better maintainability and global usage
Here's a suggested enhancement:
<script lang="ts" setup> import { useVbenModal } from '@vben/common-ui'; +import { useI18n } from 'vue-i18n'; import { message } from 'ant-design-vue'; +const { t } = useI18n(); + +// Example: Modal with animation completion callbacks const [Modal, modalApi] = useVbenModal({ onCancel() { modalApi.close(); }, onClosed() { - message.info('onClosed：关闭动画结束'); + message.info(t('modal.onClosed')); + // Example: Reset form state or clean up resources after modal is fully closed }, onConfirm() { message.info('onConfirm'); // modalApi.close(); }, onOpened() { - message.info('onOpened：打开动画结束'); + message.info(t('modal.onOpened')); + // Example: Focus on first input or load data after modal is fully opened }, }); </script>packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (1)
30-32: Document event sequence in component comments.Consider adding JSDoc comments to clarify the event sequence:
- User triggers close/open
- Animations start
- Animations complete
- opened/closed events emit
+/** + * Events emitted by the dialog: + * - close: Emitted when close is triggered + * - opened: Emitted after open animation completes + * - closed: Emitted after close animation completes + */ const emits = defineEmits< { close: []; closed: []; opened: [] } & DialogContentEmits >();Also applies to: 49-55, 68-68
docs/src/components/common-ui/vben-modal.md (1)
117-117: Consider adding example usage for the new events.The new
onClosedandonOpenedevents are well-documented with clear descriptions and version information. To further enhance the documentation, consider adding example usage in the demo sections to showcase these new lifecycle events.Example addition could look like:
const [Modal, modalApi] = useVbenModal({ onOpened: () => { console.log('Modal animation completed, now fully visible') }, onClosed: () => { console.log('Modal animation completed, now fully hidden') } })Also applies to: 120-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
docs/src/components/common-ui/vben-modal.md(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 comments (11)
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (2)
142-146: LGTM! Well-documented callback for modal close animation completion.
The onClosed callback is properly typed as optional and includes clear JSDoc documentation. This maintains backward compatibility while enhancing the modal's lifecycle events.
157-161: LGTM! Well-documented callback for modal open animation completion.
The onOpened callback is properly typed as optional and includes clear JSDoc documentation. This maintains backward compatibility while enhancing the modal's lifecycle events.
Let's verify the implementation of these callbacks:
✅ Verification successful
The onOpened callback is correctly implemented and triggered
The verification shows that:
- The callback is properly implemented in
modal-api.tswhere it safely invokes the optional callback - It's correctly wired up in
modal.vuewhere it's triggered by the@openedevent from theDialogContentcomponent - The implementation is being used correctly in the playground demo where it shows a practical example of handling the modal open animation completion
The implementation maintains a clean separation of concerns and follows proper event handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of onOpened and onClosed callbacks
# Expected: Find the actual invocation of these callbacks after animations
# Search for animation-related code that triggers these callbacks
rg -A 5 "onOpened|onClosed" --type vue
Length of output: 68
Script:
#!/bin/bash
# Let's search in all files since the modal implementation might be in TypeScript
rg -A 10 "onOpened\??\("
Length of output: 2348
Script:
#!/bin/bash
# Let's check the animation implementation to ensure the callback is triggered at the right time
rg -A 10 "@opened"
Length of output: 858
Script:
#!/bin/bash
# Let's check what component provides the @opened event
rg -B 10 "<Dialog" packages/@core/ui-kit/popup-ui/src/modal/modal.vue
Length of output: 2131
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)
30-32: LGTM! Well-structured event definitions.
The new events 'closed' and 'opened' complement the existing 'close' event, providing clear distinction between the action and state changes.
68-68: Verify animation completion timing.
The animationend event binding looks correct, but let's verify that it fires after all animations (fade, zoom, and slide) complete.
✅ Verification successful
Let me search for the specific animation classes used in the DialogContent component to understand the exact animations being applied.
Let me try a different approach to find the animation-related code in the DialogContent component.
Let me check the animation classes used in the Tailwind configuration since the component uses Tailwind's animation utilities.
Based on the DialogContent component's implementation, I can now provide a final response:
Animation completion timing is correctly handled
The component uses a combination of Tailwind's animate utilities that work together:
data-[state=open]:animate-in/data-[state=closed]:animate-outfor animation statesfade-in-0/fade-out-0for opacityzoom-in-95/zoom-out-95for scalingslide-in-from-top-[48%]/slide-out-to-top-[48%]for movement
The @animationend event will fire after the longest animation in this sequence completes, ensuring proper timing for the opened/closed events.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any animation duration or delay classes that might affect the timing
rg -g '*.css' -g '*.scss' 'animate-|fade-|zoom-|slide-' -A 2
Length of output: 10418
Script:
#!/bin/bash
# Search for the DialogContent component implementation to see animation classes
rg -g '*.vue' 'class=".*(?:fade|zoom|slide).*"' packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue -A 2
Length of output: 118
Script:
#!/bin/bash
# Get the full content of DialogContent.vue to analyze animations
cat packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue
Length of output: 2522
Script:
#!/bin/bash
# Search for animation configuration in tailwind config files
rg -g 'tailwind.config.*' -g 'theme.config.*' 'animation|keyframes' -A 5
Length of output: 74
packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (3)
9-14: LGTM: Type definition changes are well-structured.
The new event handlers are properly typed and maintain alphabetical ordering.
31-34: LGTM: Constructor parameter destructuring is correct.
The new event handlers are properly destructured from options, maintaining consistency with existing pattern.
87-90: LGTM: API initialization is consistent.
The new event handlers are properly initialized in the api object.
docs/src/components/common-ui/vben-modal.md (1)
113-114: LGTM: Well-structured table format with version information.
The addition of the version column enhances API documentation by clearly indicating when features were introduced.
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (3)
191-191: LGTM! The event handlers are implemented correctly.
The new @closed and @opened event handlers are properly implemented:
- They use optional chaining to safely handle undefined
modalApi - They're placed appropriately among other event handlers
- They align with the PR's objective of adding dialog open/close events
Also applies to: 196-196
191-191: Verify documentation coverage.
Let's ensure the new events are properly documented:
Also applies to: 196-196
✅ Verification successful
Documentation is properly maintained
The events onClosed and onOpened are documented in docs/src/components/common-ui/vben-modal.md:
onClosed: Triggered when the closing animation completesonOpened: Triggered when the opening animation completes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new events are documented
# Look for documentation of these events in markdown files
echo "Checking documentation files..."
rg -A 5 "onClosed|onOpened" --type md
Length of output: 852
Script:
#!/bin/bash
# Let's verify the documentation more thoroughly by:
# 1. Finding the component's documentation file
# 2. Checking the API documentation for these events
# Find the documentation file for this modal component
fd -e md modal
# Also check for any README files in the modal directory
fd README -p "*modal*"
# Look for JSDoc comments in the component file itself
echo "Checking component documentation..."
cat "packages/@core/ui-kit/popup-ui/src/modal/modal.vue"
Length of output: 7930
191-191: Verify the complete event handling chain.
Let's ensure the complete event handling chain is properly implemented:
Also applies to: 196-196
✅ Verification successful
Event handling chain is properly implemented
Based on the verification results:
- The DialogContent component correctly defines the
closedandopenedevents in its emits - The Modal component properly handles these events and forwards them to the optional handlers (
modalApi?.onClosed()andmodalApi?.onOpened()) - The ModalApi interface correctly includes the optional
onClosedandonOpenedhandlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the DialogContent component emits these events and modalApi interface includes these methods
# Test 1: Check if DialogContent component emits these events
echo "Checking DialogContent component for event emissions..."
ast-grep --pattern 'defineEmits([{
$$$
closed: $$$,
opened: $$$,
$$$
}])'
# Test 2: Check if modalApi interface includes these methods
echo "Checking modalApi interface definition..."
ast-grep --pattern 'interface $_ {
$$$
onClosed?: () => void;
onOpened?: () => void;
$$$
}'
Length of output: 852
Script:
#!/bin/bash
# Let's try a different approach to verify the implementation
# Check for event emissions in any Vue component
echo "Checking for event emissions in Vue components..."
rg -A 3 "emit\(['\"](?:closed|opened)" --type vue
# Check for interface definitions
echo "Checking for modalApi interface..."
rg -A 5 "interface.*Modal.*Api" --type ts
# Check for event handling in the modal component
echo "Checking modal component event handling..."
rg -B 2 -A 2 "@(?:closed|opened)" packages/@core/ui-kit/popup-ui/src/modal/modal.vue
Length of output: 1294
Script:
#!/bin/bash
# Let's check the ModalState interface and event definitions
# Check ModalState interface
echo "Checking ModalState interface..."
rg -B 2 -A 10 "interface ModalState" --type ts
# Check for event handlers in the API
echo "Checking for onClosed/onOpened handlers..."
rg -B 2 -A 2 "on(?:Closed|Opened)\?:" --type ts
# Check Dialog component for event definitions
echo "Checking Dialog component..."
fd "dialog" -t f -e vue -e ts -x rg -l "defineEmits|@closed|@opened" {}
Length of output: 2043
Script:
#!/bin/bash
# Let's check the Dialog components for event emissions
# Check DialogContent.vue for event definitions
echo "Checking DialogContent.vue for events..."
rg -B 2 -A 2 "defineEmits|emit\(['\"](?:closed|opened)" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue
# Check Dialog.vue for event handling
echo "Checking Dialog.vue for event handling..."
rg -B 2 -A 2 "(?:closed|opened)" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/Dialog.vue
# Check the complete ModalProps interface
echo "Checking ModalProps interface..."
rg -B 2 -A 10 "interface ModalProps" --type ts
Length of output: 1480
| function onAnimationEnd() { | ||
| if (props.open) { | ||
| emits('opened'); | ||
| } else { | ||
| emits('closed'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for animation events.
While the implementation is clean, consider adding error handling to ensure robustness:
function onAnimationEnd() {
+ try {
if (props.open) {
emits('opened');
} else {
emits('closed');
}
+ } catch (error) {
+ console.error('Failed to handle dialog animation end:', 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.
| function onAnimationEnd() { | |
| if (props.open) { | |
| emits('opened'); | |
| } else { | |
| emits('closed'); | |
| } | |
| } | |
| function onAnimationEnd() { | |
| try { | |
| if (props.open) { | |
| emits('opened'); | |
| } else { | |
| emits('closed'); | |
| } | |
| } catch (error) { | |
| console.error('Failed to handle dialog animation end:', error); | |
| } | |
| } |
| onClosed() { | ||
| this.api.onClosed?.(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding documentation and validation.
While the implementation is clean, consider these improvements:
- Add JSDoc documentation for consistency with other methods:
+ /**
+ * Callback triggered after the modal is fully closed
+ */
onClosed() {
this.api.onClosed?.();
}
+ /**
+ * Callback triggered after the modal is fully opened
+ */
onOpened() {
this.api.onOpened?.();
}- Consider adding animation state validation to ensure these callbacks are only triggered when appropriate:
onClosed() {
// Ensure modal is actually closed before triggering
if (!this.state.isOpen) {
this.api.onClosed?.();
}
}
onOpened() {
// Ensure modal is actually open before triggering
if (this.state.isOpen) {
this.api.onOpened?.();
}
}Also applies to: 138-140
💡 Codebase verification
Add test coverage for animation lifecycle handlers
The onClosed and onOpened handlers lack test coverage. While these handlers are properly integrated in the Vue components (modal.vue and base-demo.vue), they should be tested to ensure reliable animation lifecycle handling.
Please add test cases for:
onClosedcallback executiononOpenedcallback execution
Example test cases:
it('should call onClosed callback when provided', () => {
const onClosed = vi.fn();
const modalApiWithHook = new ModalApi({ onClosed });
modalApiWithHook.onClosed();
expect(onClosed).toHaveBeenCalled();
});
it('should call onOpened callback when provided', () => {
const onOpened = vi.fn();
const modalApiWithHook = new ModalApi({ onOpened });
modalApiWithHook.onOpened();
expect(onOpened).toHaveBeenCalled();
});🔗 Analysis chain
Verify test coverage and usage.
Please ensure:
- The new event handlers have adequate test coverage
- The handlers are properly integrated with the modal's animation lifecycle
Let's check for existing tests and usage:
Also applies to: 138-140
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage of new handlers
echo "Checking for test coverage..."
rg -l "onClosed|onOpened" --type=test
# Check for usage of new handlers
echo "Checking for usage in components..."
rg -l "onClosed|onOpened" --type=vue
Length of output: 285
Script:
#!/bin/bash
# Check for test files related to modal
echo "Searching for test files..."
fd -e test.ts -e spec.ts -e test.js -e spec.js | grep -i modal
# Check for test coverage of new handlers
echo "Checking for test coverage..."
rg "onClosed|onOpened" -A 5 -B 5 --type ts --type js | grep -i "test\|spec"
# Check for usage in components
echo "Checking for usage in components..."
rg "onClosed|onOpened" -A 5 -B 5 --type ts --type js
# Check for Vue component usage
echo "Checking for Vue component usage..."
fd -e vue | xargs rg "onClosed|onOpened" -A 5 -B 5
Length of output: 11683
Script:
#!/bin/bash
# Let's check the test file content for coverage
echo "Checking test coverage in modal-api.test.ts..."
cat "packages/@core/ui-kit/popup-ui/src/modal/__tests__/modal-api.test.ts"
Length of output: 3550
Description
为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
onClosedandonOpenedfor enhanced control over modal lifecycle events.Bug Fixes
Documentation