feat(drawer): add destroyOnClose to drawer#4153
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds a ChangesDestroy-on-Close Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
examples/sites/demos/apis/drawer.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. examples/sites/demos/pc/app/drawer/webdoc/drawer.jsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/renderless/src/drawer/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue/src/drawer/src/pc.vue (1)
17-42:⚠️ Potential issue | 🟡 MinorThe
state.key++mechanism in renderless layer is dead code.The
v-if="destroyOnClose ? state.visible : true"approach correctly handles DOM destruction. However, the renderless layer (packages/renderless/src/drawer/index.tslines 64-67) incrementsstate.key++whendestroyOnCloseis enabled and the drawer closes, but this template never binds:key="state.key"to any element.This means the
state.key++code path has no effect and is dead code. The feature still works because thev-ifdirective handles the destruction, but the renderless logic should be cleaned up to avoid confusion.💡 Recommended: Remove dead code from renderless layer
Either:
- Remove
state.key++logic frompackages/renderless/src/drawer/index.tssincev-ifhandles destruction, OR- Add
:key="state.key"to the drawer main div if key-based re-rendering is intended for some purposeOption 1 (remove dead code from
packages/renderless/src/drawer/index.ts):} else { - if (props.destroyOnClose) { - nextTick(() => state.key++) - } }And remove
stateandnextTickfromwatchVisibleparameters if no longer needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue/src/drawer/src/pc.vue` around lines 17 - 42, The renderless layer increments state.key (state.key++) in packages/renderless/src/drawer/index.ts but the template in pc.vue never uses :key="state.key", making that branch dead; either remove the state.key++ and related unused watchVisible parameters (state and nextTick) from the renderless watch logic, or wire the key into the DOM by adding :key="state.key" to the main drawer element (the div with ref="drawerBox" / class 'tiny-drawer__main')—choose one approach and update only the corresponding symbols (remove state.key++ and unused watchVisible params in drawer/index.ts OR add :key="state.key" to the drawer main div in pc.vue) so the code is consistent and dead code is eliminated.
🧹 Nitpick comments (2)
packages/renderless/src/drawer/vue.ts (1)
34-34: Implementation is correct, but supports unused functionality.The
nextTickaddition and updatedwatchVisibleinvocation are correctly implemented. However, as noted inpc.vue, thestate.key++mechanism these changes support is not utilized by the template (no:keybinding exists). Consider cleaning up if the key-based re-rendering is not intended.Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderless/src/drawer/vue.ts` at line 34, The change added support for key-based re-rendering via state.key++ and updated watchVisible/nextTick handling, but the template (e.g., in pc.vue) has no :key binding so that mechanism is unused; either remove the state.key and nextTick-related updates (references: state.key++, nextTick, watchVisible) or add a :key binding in the consuming template (pc.vue) to actually use state.key for re-rendering—pick one: delete the unused state.key++ and related nextTick logic from the drawer renderless implementation, or wire :key="state.key" into the pc.vue template so the key increment triggers a remount.examples/sites/demos/pc/app/drawer/destroy-on-close-composition-api.vue (1)
1-20: LGTM! Clean and functional demo structure.The template correctly demonstrates the destroy-on-close feature with radio controls and proper drawer integration.
♻️ Optional: Simplify visibility binding
You could simplify lines 13-14 by using
v-model:visible:<tiny-drawer title="标题" - :visible="boxVisibility" - `@update`:visible="boxVisibility = $event" + v-model:visible="boxVisibility" :destroy-on-close="value" >However, the explicit event handler pattern is also valid and may be clearer for demo purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/sites/demos/pc/app/drawer/destroy-on-close-composition-api.vue` around lines 1 - 20, Simplify the visibility binding by replacing the explicit :visible="boxVisibility" and `@update`:visible="boxVisibility = $event" pattern on the tiny-drawer with the shorthand v-model:visible="boxVisibility" so the drawer's visibility is two-way bound via boxVisibility; locate the tiny-drawer usage in the template where boxVisibility is referenced and apply the v-model:visible change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/sites/demos/pc/app/drawer/destroy-on-close.spec.ts`:
- Around line 9-15: The test currently opens the drawer but never closes it
before asserting; update the flow in the spec that uses demo.locator(...
'关闭时不销毁') / demo.getByRole('button', { name: '点击打开抽屉' }) and the referenced
drawer locator: after clicking the open button, first assert the drawer is
visible (toBeVisible or toHaveCount > 0), then close the drawer (e.g., use
demo.getByRole to click the drawer close button,
demo.locator('.ant-drawer-mask').click(), or send Escape) and only after that
assert the expected outcome — for the non-destroy case assert the drawer is
hidden, and for the destroy-on-close case assert drawer count is 0.
In `@examples/sites/demos/pc/app/drawer/webdoc/drawer.js`:
- Around line 135-148: The demo entry with demoId 'destroy-on-close' has two doc
issues: fill in name['en-US'] with a proper English label (e.g., "Destroy on
Close") and update the Chinese description desc['zh-CN'] to reference the Drawer
component instead of "Dialog-box" (replace "Dialog-box" with "drawer"). Edit the
object fields name['en-US'] and desc['zh-CN'] accordingly to keep translations
consistent.
In `@packages/renderless/src/drawer/index.ts`:
- Around line 64-67: Remove the unused and undefined state.key++ update: inside
the else branch that checks props.destroyOnClose and calls nextTick, delete the
nextTick(() => state.key++) call (and the associated increment logic) because
IDrawerState has no key property and the template already handles DOM
destruction via v-if="destroyOnClose ? state.visible : true"; keep the
surrounding conditional logic but eliminate references to state.key, nextTick
and the increment.
---
Outside diff comments:
In `@packages/vue/src/drawer/src/pc.vue`:
- Around line 17-42: The renderless layer increments state.key (state.key++) in
packages/renderless/src/drawer/index.ts but the template in pc.vue never uses
:key="state.key", making that branch dead; either remove the state.key++ and
related unused watchVisible parameters (state and nextTick) from the renderless
watch logic, or wire the key into the DOM by adding :key="state.key" to the main
drawer element (the div with ref="drawerBox" / class 'tiny-drawer__main')—choose
one approach and update only the corresponding symbols (remove state.key++ and
unused watchVisible params in drawer/index.ts OR add :key="state.key" to the
drawer main div in pc.vue) so the code is consistent and dead code is
eliminated.
---
Nitpick comments:
In `@examples/sites/demos/pc/app/drawer/destroy-on-close-composition-api.vue`:
- Around line 1-20: Simplify the visibility binding by replacing the explicit
:visible="boxVisibility" and `@update`:visible="boxVisibility = $event" pattern on
the tiny-drawer with the shorthand v-model:visible="boxVisibility" so the
drawer's visibility is two-way bound via boxVisibility; locate the tiny-drawer
usage in the template where boxVisibility is referenced and apply the
v-model:visible change.
In `@packages/renderless/src/drawer/vue.ts`:
- Line 34: The change added support for key-based re-rendering via state.key++
and updated watchVisible/nextTick handling, but the template (e.g., in pc.vue)
has no :key binding so that mechanism is unused; either remove the state.key and
nextTick-related updates (references: state.key++, nextTick, watchVisible) or
add a :key binding in the consuming template (pc.vue) to actually use state.key
for re-rendering—pick one: delete the unused state.key++ and related nextTick
logic from the drawer renderless implementation, or wire :key="state.key" into
the pc.vue template so the key increment triggers a remount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7dd37db6-245b-4407-be7d-ffd4bd69d812
📒 Files selected for processing (9)
examples/sites/demos/apis/drawer.jsexamples/sites/demos/pc/app/drawer/destroy-on-close-composition-api.vueexamples/sites/demos/pc/app/drawer/destroy-on-close.spec.tsexamples/sites/demos/pc/app/drawer/destroy-on-close.vueexamples/sites/demos/pc/app/drawer/webdoc/drawer.jspackages/renderless/src/drawer/index.tspackages/renderless/src/drawer/vue.tspackages/vue/src/drawer/src/index.tspackages/vue/src/drawer/src/pc.vue
…er_destroyOnClose
docs: update drawer destroy-on-close description
…er_destroyOnClose
|
@Yujing613 E2E测试用例报错了，有空看下哈 |
@Yujing613 The E2E test case reported an error, take a look when you have time. |
…er_destroyOnClose
3 participants
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?
Drawer 组件关闭时默认仅隐藏内容，不会销毁内部 DOM 元素。
Issue Number: N/A
What is the new behavior?
新增
destroyOnClose属性，支持在抽屉关闭时销毁内部元素，而非隐藏。Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
destroy-on-closeprop to the drawer component, allowing drawer contents to be destroyed on close instead of hidden in the DOM (default:false).Documentation