fix(dialog-box): [dialog-box] modify dialog-box of demo design bug #2382
Conversation
WalkthroughThe changes in this pull request involve modifications to the dialog box components in a Vue application. Key alterations include the replacement of Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (8)
examples/sites/demos/pc/app/dialog-box/hidden-header-composition-api.vue (2)
5-5: LGTM! Consider i18n for text content.The change from
<span>to<div>withbody-classis appropriate for block-level content.Consider using i18n for the text content "隐藏弹窗的标题部分" to support internationalization, though this might be outside the scope of this demo-specific fix.
20-24: Document the purpose of the 32px margin.While the scoped styling is well-implemented, consider adding a comment explaining why 32px was chosen as the margin-top value. This helps maintainers understand if this value is related to the hidden header's height or other layout considerations.
<style scoped> .body-class { + /* Compensates for the hidden header space */ margin-top: 32px; } </style>examples/sites/demos/pc/app/dialog-box/hidden-header.vue (1)
30-32: Consider documenting the margin-top requirement.The 32px margin-top seems specifically chosen to compensate for the hidden header. If this is a required adjustment for all hidden-header dialogs, it should be documented in the component's documentation.
Consider adding a comment explaining why this margin is necessary:
.body-class { + /* Compensate for the spacing when dialog header is hidden */ margin-top: 32px; }examples/sites/demos/pc/app/dialog-box/lock-scroll-composition-api.vue (3)
5-5: Add @close event handler to second dialog box for consistency.While adding the @close handler to the first dialog is good practice, the second dialog box should also handle the close event similarly to maintain consistent behavior.
- <tiny-dialog-box :lock-scroll="true" v-model:visible="visible2" title="消息" width="30%"> + <tiny-dialog-box :lock-scroll="true" v-model:visible="visible2" title="消息" width="30%" @close="closeDlg(false)">
Line range hint
28-47: Consider extracting scroll management logic into a composable.The scroll management logic could be reused across components. Consider creating a dedicated composable:
// useScrollLock.js import { ref, onUnmounted } from 'vue' export function useScrollLock() { const originalStyle = { overflow: document.body.style.overflow, height: document.body.style.height } const lockScroll = () => { document.body.style.overflow = 'hidden' document.body.style.height = '100vh' } const unlockScroll = () => { document.body.style.overflow = 'auto' document.body.style.height = '130vh' } onUnmounted(() => { document.body.style.overflow = originalStyle.overflow document.body.style.height = originalStyle.height }) return { lockScroll, unlockScroll } }This would simplify your component code and make the scroll management logic reusable.
Based on the gathered information, I can now provide the final response:
Direct DOM manipulation of scroll behavior should be handled by the component's internal logic
The current implementation in
lock-scroll-composition-api.vuedirectly manipulatesdocument.body.stylewhich bypasses the component's built-in scroll locking mechanism. The DialogBox component already:
- Has a built-in
lock-scrollprop that should handle scroll behavior- Uses a proper renderless architecture with
hideScrollbar/showScrollbarutilities- Implements scroll locking through a consistent class-based approach (
SCROLL_LOCK_CLASS)You should:
- Remove direct DOM manipulation from
openDlgandcloseDlgmethods- Rely solely on the
:lock-scrollprop to control scroll behavior- Let the component handle its own scroll locking internally
Example refactor:
<template> <tiny-dialog-box :lock-scroll="false" v-model:visible="visible1" ...> <tiny-dialog-box :lock-scroll="true" v-model:visible="visible2" ...> </template> <script setup> function openDlg(isScroll) { if (isScroll) { visible1.value = true } else { visible2.value = true } } function closeDlg(isScroll) { if (isScroll) { visible1.value = false } else { visible2.value = false } } </script>🔗 Analysis chain
Line range hint
1-47: Verify scroll locking implementation across components.Let's check for consistent scroll locking implementation across other dialog components.
Let me generate additional scripts to gather more specific information about scroll locking implementation.
Rationale for these scripts:
- We need to see how scroll locking is implemented in the main component
- We should compare with other lock-scroll examples to ensure consistency
- This will help us determine if the current implementation aligns with the component's design
Let me generate one final script to check the renderless implementation since the main component delegates the scroll locking logic there.
This will help us understand how the scroll locking is actually implemented in the component's logic layer and if there are any utility functions handling it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might be affected by scroll locking # Look for direct body style modifications echo "Checking for direct body style modifications:" rg "document\.body\.style\.(overflow|height)" -t vue # Look for other dialog components echo "\nChecking for other dialog components:" fd -e vue -e jsx -e tsx | rg "dialog"Length of output: 5381
Script:
#!/bin/bash # Check the main dialog-box component implementation echo "Checking main dialog-box component implementation:" cat packages/vue/src/dialog-box/src/pc.vue # Check other examples of lock-scroll usage echo "\nChecking other lock-scroll examples:" cat examples/sites/demos/pc/app/dialog-box/lock-scroll.vue cat examples/sites/demos/mobile/app/dialogbox/lock-scroll.vueLength of output: 6959
Script:
#!/bin/bash # Check the renderless implementation for scroll locking echo "Checking dialog-box renderless implementation:" cat packages/vue/src/dialog-box/src/renderless.ts || cat packages/renderless/src/dialog-box/vue.ts || cat packages/vue-renderless/dialog-box/vue.ts # Also check for any modal-related utilities that might handle scroll locking echo "\nChecking for modal-related utilities:" fd -e ts -e js | rg "modal|scroll" | xargs grep -l "lockScroll"Length of output: 5533
examples/sites/demos/pc/app/dialog-box/lock-scroll.vue (2)
5-5: Consider adding @close handler to both dialog boxes for consistency.While adding the @close event handler to the first dialog box is good practice, the second dialog box lacks this handler. This inconsistency could lead to different cleanup behaviors.
- <tiny-dialog-box :lock-scroll="true" v-model:visible="visible2" title="消息" width="30%"> + <tiny-dialog-box :lock-scroll="true" v-model:visible="visible2" title="消息" width="30%" @close="closeDlg(false)">
38-43: Consider refactoring body style management.The current implementation has several architectural concerns:
- Direct DOM manipulation is scattered across methods
- No cleanup on component unmount
- Body styles might persist if component is unmounted while dialog is open
Consider creating a dedicated scroll manager:
// scrollManager.js export const scrollManager = { enableScroll() { document.body.style.overflow = 'auto' document.body.style.height = '130vh' }, disableScroll() { document.body.style.overflow = 'hidden' document.body.style.height = '100vh' } } // In your component import { scrollManager } from './scrollManager' export default { // ... other code ... beforeUnmount() { // Cleanup scrollManager.enableScroll() }, methods: { openDlg(isScroll) { if (isScroll) { scrollManager.enableScroll() this.visible1 = true } else { scrollManager.disableScroll() this.visible2 = true } } // ... rest of the methods } }Also applies to: 46-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/sites/demos/pc/app/dialog-box/hidden-header-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/dialog-box/hidden-header.vue (2 hunks)
- examples/sites/demos/pc/app/dialog-box/lock-scroll-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/dialog-box/lock-scroll.vue (2 hunks)
🔇 Additional comments (2)
examples/sites/demos/pc/app/dialog-box/hidden-header-composition-api.vue (1)
Line range hint
1-18: Well-structured component implementation.The component follows Vue 3 best practices:
- Proper use of Composition API
- Correct v-model binding for dialog visibility
- Clean implementation of footer slot with proper close handling
examples/sites/demos/pc/app/dialog-box/hidden-header.vue (1)
5-5: Verify if margin-top compensation is necessary when header is hidden.The change from
<span>to<div class="body-class">suggests there was a spacing issue, but adding margin to compensate for a hidden header might indicate an underlying layout problem in the DialogBox component itself.
James-9696
changed the title
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?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation