Conversation
📝 WalkthroughWalkthroughAdds a new public prop Changes
Sequence DiagramsequenceDiagram
participant User
participant DialogRoot
participant DialogContent
participant Presence
participant DOM
User->>DialogRoot: Toggle open (props include unmountOnHide)
DialogRoot->>DialogContent: Provide context (unmountOnHide ref)
DialogContent->>Presence: Render with :force-mount = forceMount || !unmountOnHide
Presence->>Presence: Determine `present` (enter/leave)
Presence-->>DialogContent: Supply `present` via scoped slot
DialogContent->>DOM: Render content and apply v-show="present"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/Dialog/DialogOverlay.vue (1)
26-41:⚠️ Potential issue | 🔴 CriticalCritical: Body scroll locks immediately when
unmountOnHide=false.When
unmountOnHideisfalse, theforce-mountprop evaluates totrue(line 30:forceMount || !rootContext.unmountOnHide.value), causingDialogOverlayImplto mount immediately. However,DialogOverlayImplcallsuseBodyScrollLock(true)unconditionally on mount (line 16 of DialogOverlayImpl.vue). This locks body scroll as soon as the component renders, even when the dialog is closed and only visually hidden viav-show="present".This breaks page scrolling for any page that includes a closed dialog with
unmountOnHide=false.The scroll lock should be tied to the
presentstate rather than component mount. Consider conditionally enabling the scroll lock based on whether the dialog is open:// In DialogOverlayImpl.vue -useBodyScrollLock(true) +const scrollLocked = useBodyScrollLock(false) +watch(() => rootContext.open.value, (isOpen) => { + scrollLocked.value = isOpen +}, { immediate: true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Dialog/DialogOverlay.vue` around lines 26 - 41, The DialogOverlayImpl currently calls useBodyScrollLock(true) on mount which locks scrolling whenever force-mounting (when rootContext.unmountOnHide is false); change the behavior so the scroll lock is driven by the Presence "present" state instead of mount: pass the present state into DialogOverlayImpl (or expose it via a prop like "open" / "present") from DialogOverlay.vue and update DialogOverlayImpl to call useBodyScrollLock(present) or watch the present prop and enable/disable the lock accordingly so body scroll is only locked when present is true.
🧹 Nitpick comments (1)
docs/content/meta/DialogRoot.md (1)
24-29: Documentation description could be more informative.The description here is minimal compared to the JSDoc in
DialogRoot.vue. Consider aligning the documentation to mention the SEO/performance benefits:{ 'name': 'unmountOnHide', - 'description': '<p>When <code>true</code>, the element will be unmounted on closed state.</p>\n', + 'description': '<p>When set to <code>false</code>, the dialog content and overlay will not be unmounted when closed, but instead hidden with CSS. Useful for SEO or when you want to improve performance by not remounting the component on every open.</p>\n', 'type': 'boolean', 'required': false, 'default': 'true' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/meta/DialogRoot.md` around lines 24 - 29, Update the documentation entry for the prop 'unmountOnHide' to match the richer JSDoc in DialogRoot.vue: explain that when true the dialog's DOM is removed on close (not just "unmounted"), and note the SEO and performance benefits (reduces DOM size, prevents hidden content from being indexed, and lowers memory/paint costs). Keep the type/default/required fields the same but expand the 'description' text to include these SEO/perf details and a brief example of when to set it to false (e.g., to preserve state or for animations), referencing 'unmountOnHide' and 'DialogRoot.vue' for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/Dialog/DialogOverlay.vue`:
- Around line 26-41: The DialogOverlayImpl currently calls
useBodyScrollLock(true) on mount which locks scrolling whenever force-mounting
(when rootContext.unmountOnHide is false); change the behavior so the scroll
lock is driven by the Presence "present" state instead of mount: pass the
present state into DialogOverlayImpl (or expose it via a prop like "open" /
"present") from DialogOverlay.vue and update DialogOverlayImpl to call
useBodyScrollLock(present) or watch the present prop and enable/disable the lock
accordingly so body scroll is only locked when present is true.
---
Nitpick comments:
In `@docs/content/meta/DialogRoot.md`:
- Around line 24-29: Update the documentation entry for the prop 'unmountOnHide'
to match the richer JSDoc in DialogRoot.vue: explain that when true the dialog's
DOM is removed on close (not just "unmounted"), and note the SEO and performance
benefits (reduces DOM size, prevents hidden content from being indexed, and
lowers memory/paint costs). Keep the type/default/required fields the same but
expand the 'description' text to include these SEO/perf details and a brief
example of when to set it to false (e.g., to preserve state or for animations),
referencing 'unmountOnHide' and 'DialogRoot.vue' for context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf1570ca-06c5-4ebb-9324-ba2334be1db8
📒 Files selected for processing (4)
docs/content/meta/DialogRoot.mdpackages/core/src/Dialog/DialogContent.vuepackages/core/src/Dialog/DialogOverlay.vuepackages/core/src/Dialog/DialogRoot.vue
|
I just tested my changes in production and made small tweaks. It is working fine now with both I published it to npm: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/Dialog/DialogContentModal.vue`:
- Line 23: The disable-outside-pointer-events binding currently uses
rootContext.open.value which releases the global pointer-events lock during the
Presence exit phase; change it to use the Presence "present" state used to keep
content mounted/visible instead. Thread the parent's scoped-slot present flag
through into DialogContentModal (same pattern used for other Presence-controlled
behavior) and bind :disable-outside-pointer-events to that present value so the
body pointer-events lock stays active for the entire visible lifetime; locate
references to rootContext.open.value in DialogContentModal and the parent slot
that renders it, add a present prop/slot param, and ensure
DismissableLayer-style present-driven behavior is followed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a4b16a6-08d9-4341-ac45-5738d95c9545
📒 Files selected for processing (1)
packages/core/src/Dialog/DialogContentModal.vue
docs:Fixed typo and removed non-existent Motion prop "multiple"
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/Dialog/DialogContentModal.vue (1)
7-15:⚠️ Potential issue | 🟠 MajorGate
useHideOthersby actual visibility.With
unmountOnHide=false, this component can stay mounted after close.useHideOthers(currentElement)at Line 15 is still mount-scoped, so the rest of the page can remainaria-hiddenwhile the dialog is only hidden, which blocks screen-reader access to the underlying content. Please make that effect trackpresent/visible state instead of mount state alone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Dialog/DialogContentModal.vue` around lines 7 - 15, The call to useHideOthers(currentElement) must be gated by the dialog's visibility (props.present) so aria-hidden is applied only while visible: call useHideOthers inside a watcher on props.present (or a computed) so when present becomes true you invoke useHideOthers(currentElement) and capture its cleanup/stop function, and when present becomes false you call that cleanup (and also call it on component unmount); update DialogContentModal.vue to store and dispose the returned teardown from useHideOthers instead of invoking it unconditionally at mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/Dialog/DialogContentModal.vue`:
- Line 7: The props object currently forwards the local present prop to
DialogContentImpl causing it to leak as a fallthrough attribute; destructure
props to remove present (e.g., const { present, ...restProps } = props) and then
use v-bind with restProps and emitsAsProps when rendering DialogContentImpl so
that DialogContentImpl (and DismissableLayer) no longer receive the present
prop.
---
Outside diff comments:
In `@packages/core/src/Dialog/DialogContentModal.vue`:
- Around line 7-15: The call to useHideOthers(currentElement) must be gated by
the dialog's visibility (props.present) so aria-hidden is applied only while
visible: call useHideOthers inside a watcher on props.present (or a computed) so
when present becomes true you invoke useHideOthers(currentElement) and capture
its cleanup/stop function, and when present becomes false you call that cleanup
(and also call it on component unmount); update DialogContentModal.vue to store
and dispose the returned teardown from useHideOthers instead of invoking it
unconditionally at mount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71cc589b-910e-4cd3-99af-fb84bad7171c
📒 Files selected for processing (4)
docs/content/docs/guides/animation.mdpackages/core/src/Dialog/DialogContent.vuepackages/core/src/Dialog/DialogContentImpl.vuepackages/core/src/Dialog/DialogContentModal.vue
✅ Files skipped from review due to trivial changes (2)
- docs/content/docs/guides/animation.md
- packages/core/src/Dialog/DialogContentImpl.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Dialog/DialogContent.vue
1 participant
resolves #1727
Summary by CodeRabbit
New Features
Bug Fixes
Documentation