feat: add opened and closed events for dialog #4775
Conversation
|
WalkthroughThe pull request introduces enhancements to the Vben Modal component's documentation and functionality. It adds 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 (1)
10-12: Consider internationalizing the messages.The implementation of the new event handlers looks good and aligns with the PR objectives. However, the messages are hardcoded in Chinese. Consider using i18n translations to support multiple languages.
Example approach:
- message.info('onClosed：关闭动画结束'); + message.info(t('modal.onClosedMessage')); - message.info('onOpened：打开动画结束'); + message.info(t('modal.onOpenedMessage'));Also applies to: 17-19
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (2)
142-146: LGTM! Consider adding English documentation.The
onClosedcallback is well-defined and properly integrated. However, since this appears to be an international project, consider adding English documentation alongside the Chinese comments./** - * 弹窗关闭动画结束的回调 + * Callback triggered after the modal close animation ends + * 弹窗关闭动画结束的回调 * @returns */ onClosed?: () => void;
157-161: LGTM! Consider adding English documentation.The
onOpenedcallback is well-defined and properly integrated. However, since this appears to be an international project, consider adding English documentation alongside the Chinese comments./** - * 弹窗打开动画结束的回调 + * Callback triggered after the modal open animation ends + * 弹窗打开动画结束的回调 * @returns */ onOpened?: () => void;docs/src/components/common-ui/vben-modal.md (1)
117-117: Consider enhancing the event descriptions.While the descriptions are accurate, they could be more detailed to help developers understand the exact timing and common use cases. Consider expanding them like this:
-| 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 (1)
191-196: Well-designed event handling enhancement.The addition of
openedandclosedevents follows a clean and consistent pattern:
- Events are properly integrated with existing event handlers
- Implementation uses safe optional chaining
- Changes are minimal and focused
- No breaking changes introduced
This enhancement provides valuable hooks for animation completion while maintaining the component's existing behavior.
📜 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 comments (13)
playground/src/views/examples/modal/base-demo.vue (1)
10-19: Verify integration with useVbenModal API.
The new event handlers are correctly implemented. Let's verify that the useVbenModal API properly supports these callbacks.
✅ Verification successful
The useVbenModal API fully supports the onClosed and onOpened callbacks
The verification shows complete integration:
- Type definitions exist in
packages/@core/ui-kit/popup-ui/src/modal/modal.ts - Implementation in
packages/@core/ui-kit/popup-ui/src/modal/modal-api.tsproperly handles both callbacks - Unit tests in
modal-api.test.tsverify the functionality - The modal component in
modal.vuecorrectly triggers these callbacks - Documentation in
vben-modal.mdconfirms these are supported since version 5.4.3
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that useVbenModal API supports the new callbacks
# Test: Search for the type definitions and implementation
ast-grep --pattern 'interface ModalApiOptions {
$$$
onClosed?: $_
onOpened?: $_
$$$
}'
# Test: Search for any existing usage patterns
rg -A 2 'onClosed|onOpened'
Length of output: 5544
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
Line range hint 142-161: Verify callback implementation in ModalApi.
Let's verify that these callbacks are properly implemented in the ModalApi class.
✅ Verification successful
Let me try a different approach to find the callback implementations.
Callbacks are properly implemented and verified through tests
The implementation in modal-api.ts correctly handles all callbacks:
onOpenChange: Called when modal's open state changes (verified by test)onOpened: Called after modal opens and animation completes (verified by test)onClosed: Called after modal closes (verified by test)onConfirm: Called when confirm action is triggered
Key evidence:
- Implementation in
modal-api.tsshows proper callback invocations with appropriate state checks - Test file
modal-api.test.tscontains specific tests verifying each callback - Callbacks are properly typed and included in the ModalApi's options interface
- State management ensures callbacks are called at the correct lifecycle moments
The implementation follows best practices by:
- Checking state conditions before invoking callbacks
- Using TypeScript for type safety
- Including comprehensive test coverage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of onOpened and onClosed callbacks in ModalApi
# Test: Search for the implementation of these callbacks
echo "Searching for callback implementations..."
ast-grep --pattern 'onOpened($$$)' -l ts
ast-grep --pattern 'onClosed($$$)' -l ts
# Test: Check for animation-related code that might trigger these callbacks
echo "Searching for animation-related code..."
rg -l "animation" --type ts
Length of output: 605
Script:
#!/bin/bash
# Search for modal-related files and their content
echo "Searching for modal implementation files..."
fd "modal" -e ts -e tsx
echo "\nChecking modal implementation for callback usage..."
rg "onOpened|onClosed|onConfirm|onOpenChange" -A 5 --type ts
echo "\nSearching for Modal class/interface definitions..."
ast-grep --pattern 'class Modal {
$$$
}'
ast-grep --pattern 'interface Modal {
$$$
}'
Length of output: 14398
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (3)
30-32: LGTM: Event definitions are well-typed and follow Vue best practices.
The new events 'closed' and 'opened' are properly defined with TypeScript types while maintaining the existing 'close' event.
68-68: LGTM: Event binding is correctly implemented.
The animationend event is properly bound to the DialogContent component and will capture all animation completions.
49-55: Implementation looks good, but verify animation states.
The animation end handler correctly emits events based on the dialog state. However, we should ensure it works correctly when animations are disabled via CSS.
Let's check if there are any CSS rules that might disable animations:
✅ Verification successful
Let me verify if there are any animation-related styles directly affecting the Dialog component:
Let me check the Dialog component's animation implementation to understand how animations are handled:
Animation handling is properly implemented with fallbacks
The implementation is robust as it uses Vue's <Transition> component with CSS classes for animations (animate-in, animate-out, fade-in-0, etc.) and properly handles animation end states through the @animationend event. Even if animations are disabled via CSS or prefers-reduced-motion, the component will still emit the correct events since the transition end events are guaranteed to fire by Vue's transition system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSS rules that might disable animations
rg -g "*.{css,scss,vue}" "animation.*none|transition.*none|prefers-reduced-motion"
Length of output: 4051
Script:
#!/bin/bash
# Search for animation-related styles in Dialog component files
rg -g "*.{css,scss,vue}" "animation|transition" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
Length of output: 738
Script:
#!/bin/bash
# Check Dialog component's animation implementation
ast-grep --pattern 'class DialogContent {
$$$
}'
# Also check for any animation-related CSS classes
rg -g "*.{css,scss,vue}" "animate|fade|slide" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
Length of output: 960
packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (4)
9-14: LGTM: API interface updates are well-structured.
The new callback types are properly integrated into the API interface while maintaining alphabetical ordering.
31-34: LGTM: Constructor parameter updates are consistent.
The new callbacks are properly destructured from the options object, maintaining consistency with the API interface.
87-90: LGTM: API initialization is properly implemented.
The new callbacks are correctly initialized in the api object, maintaining consistency with the interface.
127-134: 🛠️ Refactor suggestion
Consider improving the reliability of animation callbacks.
The current implementation of onClosed and onOpened has potential issues:
- The state checks might allow multiple callback invocations while the modal remains in the same state.
- There's no error handling for callbacks, which could affect the modal's stability.
- The documentation could be clearer about the exact timing of these callbacks.
Consider applying these improvements:
/**
- * 弹窗关闭动画播放完毕后的回调
+ * Callback fired after the modal's closing animation completes.
+ * This is called exactly once per close transition.
*/
onClosed() {
- if (!this.state.isOpen) {
+ try {
this.api.onClosed?.();
+ } catch (error) {
+ console.error('Error in onClosed callback:', error);
}
}
/**
- * 弹窗打开动画播放完毕后的回调
+ * Callback fired after the modal's opening animation completes.
+ * This is called exactly once per open transition.
*/
onOpened() {
- if (this.state.isOpen) {
+ try {
this.api.onOpened?.();
+ } catch (error) {
+ console.error('Error in onOpened callback:', error);
}
}The state checks were removed because:
- These methods should be called by the animation system exactly once when the respective animation completes
- The current state checks don't prevent multiple invocations while in the same state
- If timing control is needed, it should be handled by the animation system that calls these methods
Let's verify how these callbacks are invoked:
Also applies to: 143-150
docs/src/components/common-ui/vben-modal.md (1)
113-114: LGTM: Well-structured table format with version tracking.
The addition of the version column is a good practice for tracking API evolution and maintaining backward compatibility awareness.
packages/@core/ui-kit/popup-ui/src/modal/__tests__/modal-api.test.ts (1)
113-127: Test structure and organization looks good!
The new test cases follow the established patterns in the test suite and maintain proper test isolation.
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (2)
191-191: LGTM: Safely implemented closed event handler.
The implementation correctly uses optional chaining and follows Vue.js event handling patterns.
196-196: LGTM: Verify event timing with animation completion.
The implementation is correct. However, let's verify that these events are triggered after the animation completes.
Let's check the DialogContent component implementation to confirm the event timing:
✅ Verification successful
Let me try another search strategy to find the event emission logic.
Let me search for the DialogContent component to verify the animation timing.
Based on the DialogContent component implementation found, I can now provide the final response:
✅ Event timing is correctly implemented with animations
The implementation correctly emits the opened and closed events after animations complete through the onAnimationEnd handler. The component uses CSS animations with proper state transitions (data-[state=open] and data-[state=closed]) and the events are emitted only after these animations finish.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that opened/closed events are emitted after animation completion
# Look for animation-related code and event emission in DialogContent
# Search for animation-related code in DialogContent
rg -A 5 "transition|animation.*opened|animation.*closed" packages/@core/ui-kit/popup-ui/src
# Search for event emission in DialogContent
ast-grep --pattern 'emit($_, "opened")'
ast-grep --pattern 'emit($_, "closed")'
Length of output: 1542
Script:
#!/bin/bash
# Search for opened/closed event handling in modal component and related files
rg -A 5 -B 5 "onOpened|onClosed|@opened|@closed" packages/@core/ui-kit/popup-ui/src
# Search for dialog/modal related components that might handle these events
fd "dialog|modal" packages/@core/ui-kit/popup-ui/src --exec cat {}
Length of output: 32880
Script:
#!/bin/bash
# Search for DialogContent component implementation
fd "DialogContent" packages/@core/ui-kit --exec cat {}
# Search for animation/transition related code in shadcn-ui components
rg -A 5 "transition|animation" packages/@core/shadcn-ui/src
Length of output: 2640
Description
为Dialog新增
opened和closed事件，在打开和关闭的动画播放完毕后触发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
onClosedandonOpenedfor improved modal lifecycle management.onClosedandonOpenedcallbacks are functioning correctly.