fix(dialog-box): add before-close prop and event's doc #3775
Conversation
WalkthroughAdds a before-close interception to dialog-box: new prop and event signatures, demo components (options + composition API), a Playwright test, a webdoc demo entry, and renderless core changes (emit before-close earlier and expose 'hide'). Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant DialogBox
participant RenderlessAPI
participant BeforeCloseHandler
User->>DialogBox: trigger close (Cancel/Confirm)
DialogBox->>RenderlessAPI: handleClose(type)
RenderlessAPI->>BeforeCloseHandler: emit 'before-close' (event, hideFn)
alt intercepted (event.preventDefault or returns false)
BeforeCloseHandler-->>RenderlessAPI: intercepted
RenderlessAPI-->>DialogBox: abort close (no DOM adjustments)
DialogBox-->>User: remains open
else allowed
BeforeCloseHandler-->>RenderlessAPI: allowed
RenderlessAPI->>RenderlessAPI: call hide() and apply DOM adjustments
RenderlessAPI-->>DialogBox: closed
DialogBox-->>User: closed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
examples/sites/demos/pc/app/dialog-box/before-close.spec.ts (1)
3-6: Consider expanding test coverage for before-close behavior.The current test provides basic smoke testing by navigating to the route and checking for page errors. Consider adding assertions to verify the before-close interception functionality, such as:
- Verifying the dialog opens when a button is clicked
- Verifying the dialog remains open when closure is intercepted
- Verifying the dialog closes when interception allows it
examples/sites/demos/pc/app/dialog-box/before-close.vue (1)
39-62: Consider more predictable demo behavior.The demo uses
Math.random() > 0.5to determine whether to allow closure, which creates non-deterministic behavior that may confuse users trying to understand the feature. Consider one of these alternatives:
- Use a counter-based approach (e.g., "closes on 3rd attempt")
- Add a checkbox to let users control whether interception occurs
- Show both outcomes with separate buttons
This would make the demo more educational and easier to test.
examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue (1)
33-56: Consider more predictable demo behavior.Similar to the Options API demo, the random behavior (
Math.random() > 0.5) may confuse users. Consider using a more deterministic pattern for educational clarity, such as:
- Counter-based interception ("closes on 3rd attempt")
- User-controlled toggle for interception
- Separate buttons demonstrating allowed vs. blocked closure
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/sites/demos/apis/dialog-box.js(1 hunks)examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue(1 hunks)examples/sites/demos/pc/app/dialog-box/before-close.spec.ts(1 hunks)examples/sites/demos/pc/app/dialog-box/before-close.vue(1 hunks)examples/sites/demos/pc/app/dialog-box/webdoc/dialog-box.js(1 hunks)packages/renderless/src/dialog-box/index.ts(1 hunks)packages/renderless/src/dialog-box/vue.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/renderless/src/dialog-box/index.ts (1)
packages/renderless/src/dialog-box/vue.ts (1)
api(47-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (4)
packages/renderless/src/dialog-box/vue.ts (1)
51-51: LGTM! Public API expansion for programmatic closure.Adding
'hide'to the exported API enables programmatic dialog closure from refs, which aligns with the before-close interception pattern demonstrated in the new demos.packages/renderless/src/dialog-box/index.ts (1)
197-199: LGTM! Before-close event validation moved to early guard position.The new early guard validates the
before-closeevent before any DOM adjustments occur. TheemitEventcall correctly passesapi.hideas thehideFnparameter, enabling event handlers to programmatically close the dialog.examples/sites/demos/apis/dialog-box.js (2)
293-304: LGTM! Before-close prop documentation added.The new
before-closeprop is properly documented with correct type signature and clear description of its interception behavior.
308-316: LGTM! Before-close event signature updated.The event signature is correctly updated to reflect the new
(event, hideFn) => voidpattern, and the description clearly explains the interception mechanism usingevent.preventDefault().
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue (1)
45-56: Consider validating the hideFn parameter.While the
hideFncallback is provided by the framework and should be reliable, adding a defensive check can prevent issues if the API changes or is misused.function onBeforeClose(event, hideFn) { // 模拟异步校验，是否需要手动关闭 setTimeout(() => { if (Math.random() > 0.5) { - hideFn() // 手动关闭，使用 box2.value = false 同样效果 + hideFn?.() // 手动关闭，使用 box2.value = false 同样效果 } else { Modal.alert('随机值过小，校验失败') } }, 1000) event.preventDefault() // 拦截关闭 }
Same race condition pattern as box1.
This function has the same rapid-click race condition as
beforeCloseProp. Consider applying similar state tracking if needed for production use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue(1 hunks)examples/sites/demos/pc/app/dialog-box/before-close.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/sites/demos/pc/app/dialog-box/before-close.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (3)
examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue (3)
1-22: LGTM! Template structure is well-organized.The template correctly demonstrates both prop-based and event-based interception patterns with clear UI elements and proper bindings.
25-31: LGTM! Clean imports and state setup.All imports are used appropriately, and reactive state is properly declared.
33-44: The null safety concern is not applicable—no guard needed.The
box1Refis guaranteed to be populated whenbeforeClosePropis called because:
- The ref is bound in the template to an active component (
<tiny-dialog-box ref="box1Ref" ... >)- The
beforeClosehook only fires when the dialog is visible (controlled byv-model:visible="box1")- Other methods in the same file (e.g.,
handleBox1Closeon line 59) accessbox1Ref.valuedirectly without optional chainingThe race condition concern is valid but represents an edge case appropriate for manual verification in this demo context.
Likely an incorrect or invalid review comment.
| </div> | ||
| </template> | ||
|
|
||
| <script setup lang="jsx"> |
There was a problem hiding this comment.
Remove incorrect lang="jsx" attribute.
This file contains no JSX syntax, only standard Vue composition API code. The lang="jsx" attribute is misleading and should be removed.
Apply this diff:
-<script setup lang="jsx">
+<script setup>📝 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.
| <script setup lang="jsx"> | |
| <script setup> |
🤖 Prompt for AI Agents
In examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue
around line 24, the <script setup lang="jsx"> attribute is incorrect because the
file uses plain Vue Composition API, so remove the lang="jsx" portion from the
<script setup> tag (make it just <script setup>) to avoid misleading tooling and
linting; update the opening script tag accordingly and keep the rest of the file
unchanged.
| function handleBox1Close() { | ||
| box1Ref.value.handleClose() | ||
| } | ||
| function handleBox2Close() { | ||
| box2Ref.value.handleClose() | ||
| } |
There was a problem hiding this comment.
Add null safety checks for ref access.
Both functions directly access .value.handleClose() without null checks. This will throw a runtime error if the refs haven't been initialized.
Apply this diff:
function handleBox1Close() {
- box1Ref.value.handleClose()
+ box1Ref.value?.handleClose()
}
function handleBox2Close() {
- box2Ref.value.handleClose()
+ box2Ref.value?.handleClose()
}📝 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 handleBox1Close() { | |
| box1Ref.value.handleClose() | |
| } | |
| function handleBox2Close() { | |
| box2Ref.value.handleClose() | |
| } | |
| function handleBox1Close() { | |
| box1Ref.value?.handleClose() | |
| } | |
| function handleBox2Close() { | |
| box2Ref.value?.handleClose() | |
| } |
🤖 Prompt for AI Agents
In examples/sites/demos/pc/app/dialog-box/before-close-composition-api.vue
around lines 58 to 64, the functions call box1Ref.value.handleClose() and
box2Ref.value.handleClose() directly which can throw if the refs or their .value
are null/undefined; update each handler to guard that the ref and .value exist
and that handleClose is a function before invoking (e.g., check if box1Ref &&
box1Ref.value && typeof box1Ref.value.handleClose === 'function' then call it,
or use optional chaining and a type-safe call), ensuring no runtime error when
refs are uninitialized.
PR
添加 before-close的属性的文档
添加before-close的综合示例
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?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
New Examples
Documentation
Tests
New Public API