feat: drawer and modal support destroy on close #5268
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/@core/ui-kit/popup-ui/src/drawer/__tests__/drawer-api.test.tsOops! Something went wrong! :( ESLint: 9.17.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThis pull request introduces enhancements to the Vben Drawer and Modal components, focusing on improved lifecycle management and event handling. The changes include new properties like Changes
Sequence DiagramsequenceDiagram
participant User
participant Drawer
participant ConnectedComponent
User->>Drawer: Open Drawer
Drawer->>ConnectedComponent: Render Component
Drawer->>Drawer: Trigger onOpened Event
User->>Drawer: Close Drawer
alt destroyOnClose is true
Drawer->>ConnectedComponent: Destroy Component
Drawer->>Drawer: Recreate Component
end
Drawer->>Drawer: Trigger onClosed Event
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (8)
docs/src/components/common-ui/vben-modal.md (1)
63-63: Communicate potential performance impact ofdestroyOnClose.While destroying and recreating the component guarantees a fresh state, repeatedly toggling this feature for frequently opened modals could impact performance. Include a note advising developers to use this option selectively for heavy components.
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue (1)
35-37: Consolidate event naming to avoid confusion.
You have "close", "closed", and "opened" events, potentially causing confusion. It may be clearer to rename "close" to "requestClose" if it's meant to emit a request or user action, and keep "opened"/"closed" solely for animation completion.packages/@core/ui-kit/popup-ui/src/drawer/use-drawer.ts (1)
43-47: Possible optimization for re-creation logic.
If performance becomes a concern in frequent open-close scenarios, consider persistent instantiation with a “reset” method. Currently, this approach is fully valid, but a future refactor might reduce overhead.packages/@core/ui-kit/popup-ui/src/drawer/__tests__/drawer-api.test.ts (2)
113-119: Test coverage appears solid for the onClosed callback.You're correctly verifying that the callback is invoked. One suggestion to strengthen the test is to simulate the actual
close()operation and check ifonClosedis eventually triggered, ensuring the callback is tied to real drawer behavior rather than a direct method call.
121-127: Test coverage appears solid for the onOpened callback.Similar to the onClosed callback test, consider explicitly tying
onOpenedto the actual draweropen()flow and waiting for any post-animation triggered events to ensure your callback is invoked in a realistic scenario.docs/src/components/common-ui/vben-drawer.md (1)
79-80: Accurate parameter table updates.Both
connectedComponentanddestroyOnCloseare well documented. Including best-use cases or examples could further guide users.packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (2)
142-142: Consider verifyingdrawerApiexistence before invoking its method.While the optional chaining operator (
?.) prevents errors, it might be helpful to either log a warning or provide a fallback behavior ifdrawerApioronClosedis undefined, so that the application's internal flow remains consistent with your expectations.
Line range hint
68-68: Minor typo in function name "handerOpenAutoFocus".Beyond the changed lines, there's a small spelling discrepancy in the function name
handerOpenAutoFocus; consider renaming tohandlerOpenAutoFocusfor consistency and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/src/components/common-ui/vben-drawer.md(3 hunks)docs/src/components/common-ui/vben-modal.md(2 hunks)packages/@core/ui-kit/popup-ui/src/drawer/__tests__/drawer-api.test.ts(1 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts(4 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts(3 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue(1 hunks)packages/@core/ui-kit/popup-ui/src/drawer/use-drawer.ts(4 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.ts(1 hunks)packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts(5 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue(4 hunks)
🔇 Additional comments (18)
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
146-149: Ensure property naming consistency and usage clarity.
The destroyOnClose property name clearly conveys its purpose. However, make sure this naming is consistent in both Modal and Drawer components (e.g., if the Drawer uses the same property behavior, keep the property name the same to reduce confusion). Nicely documented feature.
packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts (4)
29-29: Good approach for re-creating the component instance.
Using isModalReady allows you to effectively reset the connected component by toggling its render state between unmount and remount. This is a clean and simple mechanism to handle dynamic initialization.
39-43: Use of nextTick is appropriate.
Calling await nextTick() ensures a proper teardown before re-mounting the component. This prevents race conditions or mismatched states during rapid open/close cycles.
52-52: Fallback element usage is a neat solution.
Rendering a static 'div' whenever isModalReady.value is false is an effective fallback preventing Vue from preserving the previous component instance. This helps rectify state inconsistencies.
80-87: Properly wire up destroyOnClose logic.
By calling injectData.reCreateModal?.() within the onClosed handler, the modal lifecycle is seamlessly reset, reconstructing the connected component only when needed. This follows the destroyOnClose contract well.
docs/src/components/common-ui/vben-modal.md (1)
85-86: Documentation properly reflects new parameters.
The newly added entries for connectedComponent and destroyOnClose are clearly explained, aligning well with the actual implementation in the modal. Good job on keeping docs up to date!
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue (2)
64-74: Confirm that onAnimationEnd is triggered only on the intended element.
This logic for checking event.target === contentRef.value?.$el is good, but if there are nested animated elements, you might accidentally miss or catch other animations. You might consider scoping your CSS animations or verifying they match a specific animation name.
83-86: Ensure template references align with composition.
The ref="contentRef" and the @animationend="onAnimationEnd" usage are consistent. This is well-structured, so no immediate changes needed.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (2)
129-129: Updated comment to “独立的抽屉组件” is appropriate.
Renaming the comment from “独立的弹窗组件” to “独立的抽屉组件” adds clarity.
132-135: Evaluate synergy between destroyOnClose, onClosed, and onOpened.
These properties/events strengthen the drawer’s lifecycle. Verify that they’re used consistently in external components to fully leverage the destroy-on-close logic.
Also applies to: 145-149, 160-164
packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts (3)
9-14: The additional callbacks in the api pick.
Adding onClosed and onOpened broadens the API but ensure that if users rely on the older approach of hooking into onOpenChange, it still aligns with these new events.
118-126: Validate that onClosed is invoked from outside.
The method checks !this.state.isOpen before calling the user’s callback. Confirm that you always call drawerApi.onClosed() once the closed animation ends in the UI layer, so the method runs as intended.
134-142: onOpened callback structures the open lifecycle.
The logic to call onOpened only if this.state.isOpen is good. This consistent approach matches the onClosed method.
packages/@core/ui-kit/popup-ui/src/drawer/use-drawer.ts (2)
33-33: isDrawerReady approach is sensible.
Toggling isDrawerReady to false, waiting a tick, then resetting it is a straightforward technique to force re-creation. It cleanly handles dynamic component usage.
81-87: destroyOnClose logic usage
The onClosed override properly re-creates the connected component if destroyOnClose is set. This is a good pattern to ensure resources are freed when the drawer closes.
docs/src/components/common-ui/vben-drawer.md (2)
57-57: Good clarity on the destroyOnClose feature.
This documentation succinctly explains how destroyOnClose resets internal states. It might be helpful to highlight any potential performance impact if the connected component is resource-intensive.
114-121: Concise explanation of new events.
The newly introduced onClosed and onOpened events are clearly documented. Consider adding a small example snippet to illustrate how developers might use these callbacks.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)
147-147: Ensure robust testing for lifecycle events.
Invoking drawerApi?.onOpened() is sound for hooking custom logic. Please confirm that all relevant scenarios—especially concurrent toggling under rapid user interaction—have test coverage to ensure the event is reliably fired only after the drawer is fully opened.
Description
1、Drawer添加
onOpened和onClosed，分别在打开动画播放完毕和关闭动画播放完毕后被调用；2、为
useVbenDrawer和useVbenModal添加destroyOnClose选项，用于控制是否在关闭后销毁并重新创建connectedComponent3、补充Drawer和Modal一些文档
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
Release Notes
New Features
destroyOnCloseoption for Drawer and Modal componentsonClosedandonOpenedlifecycle events for Drawer and Modal componentsDocumentation
Improvements