fix(vue-popup): fix v-modal animation is not working #3701
Conversation
WalkthroughReplaced the modal animation control when opening a popup: the call to PopupManager.openModal now uses the inverse of a new noAnimation prop (!props.noAnimation) instead of props.modalFade. No exported API signatures were altered. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant VC as Vue Component
participant PM as PopupManager
VC->>PM: openModal(id, zIndex, dom, modalClass, !noAnimation)
Note over VC,PM: Animation is enabled when noAnimation is false/undefined<br/>and disabled when noAnimation is true
PM-->>VC: Modal opened with specified animation setting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (2)
packages/vue/src/dialog-box/src/pc.vue (1)
21-21: Prefer object-style class binding to avoid empty string entriesUse an object for the conditional class for clarity and to avoid adding a blank class when
modalis false.- :class="['tiny-dialog-box__wrapper', dialogClass, modal ? 'has-modal' : '']" + :class="['tiny-dialog-box__wrapper', dialogClass, { 'has-modal': modal }]"packages/theme/src/dialog-box/index.less (1)
38-41: Mask-on-wrapper looks good; consider alias + property specificity
- Add an
is-modalalias for consistency with existingis-*modifiers while keepinghas-modalfor backward compatibility.- Use
background-colorinstead ofbackgroundto avoid resetting other background-* properties.- &.has-modal { - background: var(--tv-DialogBox-mask-bg-color); - } + &.has-modal, + &.is-modal { + background-color: var(--tv-DialogBox-mask-bg-color); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/dialog-box/index.less(1 hunks)packages/vue/src/dialog-box/src/pc.vue(1 hunks)
🔇 Additional comments (1)
packages/theme/src/dialog-box/index.less (1)
38-41: Verify overlay layering with.v-modalSince
.v-modalstill exists (now transparent), confirm click/close works as expected and that the wrapper’s fade animates the mask visually:
- modal=true: clicking outside dialog closes.
- modal=false: clicking outside doesn’t close.
- With/without
appendToBodyandrightSlide.If issues arise, consider lowering
.v-modalz-index beneath the wrapper or disabling its pointer events.
gausszhou
changed the title
gausszhou
force-pushed
the
zcg/issue-3636
branch
from
a1f3a47 to
3611bfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/vue/src/dialog-box/src/pc.vue (3)
21-25: Non-modal UX: ensure wrapper doesn’t block page whenmodal=false.If the wrapper covers the viewport regardless of
modal, consider CSS ensuring only.is-modalcreates a full-screen, pointer-catching layer (e.g., pointer-events: auto only when.is-modal, and.tiny-dialog-boxkeeps pointer-events: auto).If needed, adjust theme CSS:
/* mask only when modal */ .tiny-dialog-box__wrapper:not(.is-modal) { background: transparent; pointer-events: none; } .tiny-dialog-box__wrapper:not(.is-modal) .tiny-dialog-box { pointer-events: auto; }
27-47: Add dialog ARIA semantics.Improve accessibility by declaring role and labelling when modal.
Apply:
- <div + <div ref="dialog" v-show="visible" v-if="destroyOnClose ? visible : true" :class="[ { 'is-fullscreen': state.isFull, 'is-center': center, 'is-right-slide': rightSlide } ]" :style="state.style" class="tiny-dialog-box" data-tag="tiny-dialog-box" :data-dialog-box-draggable="draggable" + role="dialog" + :aria-modal="modal" + :aria-labelledby="title ? ('tiny-dialog-title-' + state.key) : undefined" :key="state.key" > - <div v-if="showHeader" ref="header" class="tiny-dialog-box__header" @mousedown="handleDrag"> + <div v-if="showHeader" ref="header" class="tiny-dialog-box__header" @mousedown="handleDrag"> <slot name="title"> - <span class="tiny-dialog-box__title">{{ title }}</span> + <span class="tiny-dialog-box__title" :id="'tiny-dialog-title-' + state.key">{{ title }}</span> </slot>
21-21: Docs and tests follow-up.Update docs to note mask now comes from
.tiny-dialog-box__wrapper.is-modal. Add an e2e asserting:
modal=true→ wrapper hasis-modal, overlay animates, clicking mask closes (if enabled).modal=false→ no mask, underlying page remains interactive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/theme-saas/src/dialog-box/index.less(0 hunks)packages/theme/src/dialog-box/index.less(1 hunks)packages/vue/src/dialog-box/src/pc.vue(1 hunks)
💤 Files with no reviewable changes (1)
- packages/theme-saas/src/dialog-box/index.less
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/dialog-box/index.less
🔇 Additional comments (2)
packages/vue/src/dialog-box/src/pc.vue (2)
21-21: Good fix: move mask onto wrapper viais-modal.Binding
['tiny-dialog-box__wrapper', dialogClass, { 'is-modal': modal }]aligns with theme changes and lets the wrapper’s transition animate the mask.
21-25: ```shell
#!/bin/bash
set -euo pipefail
echo "1) Search for modal-related symbols across .vue/.js/.ts files..."
rg -n -C3 -S -e 'v-modal' -e '.v-modal' -e 'PopupManager' -e 'modalAppendToBody' -e 'closeOnClickModal' -g '!node_modules' -g '/*.vue' -g '/.js' -g '**/.ts' -g '/*.jsx' -g '/*.tsx' || trueecho
echo "2) Show files under packages/vue/src/dialog-box to confirm paths..."
rg -n -S --hidden -g 'packages/vue/' -g 'packages//dialog-box/**' -e '' || trueecho
echo "3) Print packages/vue/src/dialog-box/src/pc.vue (lines 1-240) if it exists..."
if [ -f "packages/vue/src/dialog-box/src/pc.vue" ]; then
sed -n '1,240p' packages/vue/src/dialog-box/src/pc.vue || true
else
echo "File not found: packages/vue/src/dialog-box/src/pc.vue"
fi</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
gausszhou
force-pushed
the
zcg/issue-3636
branch
from
3611bfb to
e982b6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/sites/src/App.vue(1 hunks)packages/theme-saas/src/dialog-box/index.less(0 hunks)packages/theme/src/dialog-box/index.less(1 hunks)packages/vue/src/dialog-box/src/pc.vue(1 hunks)
💤 Files with no reviewable changes (1)
- packages/theme-saas/src/dialog-box/index.less
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/vue/src/dialog-box/src/pc.vue
- packages/theme/src/dialog-box/index.less
🔇 Additional comments (1)
examples/sites/src/App.vue (1)
63-82: Keyframe redefinitions: avoid long-term duplication; ensure build order makes this win.Verification failed — ripgrep returned "No files were searched". Re-run detection with:
rg -nP --hidden --no-ignore '@Keyframes\s+dialog-fade-(in|out)' -C1
Confirm whether dialog-fade-in/out in examples/sites/src/App.vue (lines 63–82) are temporary overrides for @opentiny/next-remoter; if so add a TODO to remove once the dependency is updated and ensure this CSS is loaded after any conflicting definitions.
examples/sites/src/App.vue
Outdated
| <style lang="less"> | ||
| // 由于 @opentiny/next-remoter/dist/style.css 中包含了旧版的弹窗和动画样式，这里额外处理一次 | ||
| .v-modal { | ||
| background: transparent; | ||
| } | ||
|
|
||
| .tiny-dialog-box__wrapper.is-modal { | ||
| background: var(--tv-DialogBox-mask-bg-color); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Scope the global .v-modal override to the docs shell to avoid collateral regressions.
Overriding .v-modal globally makes every overlay (dialogs, drawers, message boxes) transparent across the site. Limit the override to this app container and add a fallback for the mask color.
Apply within this hunk:
-.v-modal {
+.site-docs .v-modal {
background: transparent;
}
-.tiny-dialog-box__wrapper.is-modal {
- background: var(--tv-DialogBox-mask-bg-color);
+.site-docs .tiny-dialog-box__wrapper.is-modal {
+ background: var(--tv-DialogBox-mask-bg-color, rgba(0, 0, 0, 0.45));
}And outside this hunk, add a namespacing class to the root container:
- <div class="hp100 of-hidden">
+ <div class="hp100 of-hidden site-docs">Verification script (checks for other overlays used in examples and presence of the mask var):
🏁 Script executed:
#!/bin/bash
# Find usages of overlays/components that rely on .v-modal in examples
rg -nP -C2 '(Tiny(Dialog|Modal|Drawer)|tiny-(dialog-box|modal)|v-modal|MessageBox|Confirm|Drawer)' examples/sites
# Ensure the mask color var is defined somewhere in themes
rg -nP --type=css --type=less --type=vue --glob '!**/node_modules/**' --color=never '\-\-tv-DialogBox-mask-bg-color'Length of output: 270166
Scope .v-modal to the docs shell and add a fallback mask color.
Global .v-modal in examples/sites/src/App.vue (CSS hunk ≈ lines 53–61) affects many overlays; add the namespacing class to the root container and use a safe fallback for the mask var.
Apply this patch:
-.v-modal {
+.site-docs .v-modal {
background: transparent;
}
-.tiny-dialog-box__wrapper.is-modal {
- background: var(--tv-DialogBox-mask-bg-color);
+.site-docs .tiny-dialog-box__wrapper.is-modal {
+ background: var(--tv-DialogBox-mask-bg-color, rgba(0, 0, 0, 0.45));
}And add the namespacing class to the root container:
- <div class="hp100 of-hidden">
+ <div class="hp100 of-hidden site-docs">File: examples/sites/src/App.vue — update the CSS hunk (≈53–61) and the root
📝 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.
| <style lang="less"> | |
| // 由于 @opentiny/next-remoter/dist/style.css 中包含了旧版的弹窗和动画样式，这里额外处理一次 | |
| .v-modal { | |
| background: transparent; | |
| } | |
| .tiny-dialog-box__wrapper.is-modal { | |
| background: var(--tv-DialogBox-mask-bg-color); | |
| } | |
| <style lang="less"> | |
| // 由于 @opentiny/next-remoter/dist/style.css 中包含了旧版的弹窗和动画样式，这里额外处理一次 | |
| .site-docs .v-modal { | |
| background: transparent; | |
| } | |
| .site-docs .tiny-dialog-box__wrapper.is-modal { | |
| background: var(--tv-DialogBox-mask-bg-color, rgba(0, 0, 0, 0.45)); | |
| } |
🤖 Prompt for AI Agents
In examples/sites/src/App.vue around lines 53–62, the global .v-modal selector
and the mask color usage are too broad and lack a fallback; update the CSS to
scope .v-modal under a namespacing class on the root (e.g., .docs-shell
.v-modal) and change the .tiny-dialog-box__wrapper.is-modal rule to use the CSS
variable with a safe fallback value (e.g., var(--tv-DialogBox-mask-bg-color,
rgba(...))). Then add that same namespacing class to the root <div> in the
template so the styles only apply inside the docs shell.
e982b6a to
820049f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/sites/src/main.js (2)
5-6: Keep the CSS reset first; load legacy CSS immediately after it.Resets should be imported before any library CSS to avoid unintended overrides. Swap these two imports.
-import '@opentiny/next-remoter/dist/style.css' -import '@unocss/reset/eric-meyer.css' +import '@unocss/reset/eric-meyer.css' +import '@opentiny/next-remoter/dist/style.css'
4-4: Clarify and internationalize the comment.Avoid “不确定…请评估” in committed code; make the rationale explicit and add English for OSS readers.
-// 由于 @opentiny/next-remoter/dist/style.css 中包含了旧版的弹窗和动画样式，这里调整到最前，不确定是否会有其他影响，请评估 +// 为兼容示例页中的旧版弹窗/动画样式，需引入 next-remoter 的旧样式，并将其尽量靠前（在 reset 之后）以降低覆盖面。 +// Load legacy dialog/modal styles for demo compatibility; import early (right after the CSS reset) to minimize overrides. See issue #3636.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/sites/src/App.vue(0 hunks)examples/sites/src/main.js(1 hunks)packages/theme-saas/src/dialog-box/index.less(0 hunks)packages/theme/src/dialog-box/index.less(1 hunks)packages/vue/src/dialog-box/src/pc.vue(1 hunks)
💤 Files with no reviewable changes (2)
- packages/theme-saas/src/dialog-box/index.less
- examples/sites/src/App.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/vue/src/dialog-box/src/pc.vue
- packages/theme/src/dialog-box/index.less
🔇 Additional comments (1)
examples/sites/src/main.js (1)
4-5: Verify legacy .v-modal styles aren't reintroduced by @opentiny/next-remoter importFound .v-modal and dialog-fade keyframes in:
- packages/theme/src/dialog-box/index.less
- packages/theme-saas/src/dialog-box/index.less
node_modules/@opentiny/next-remoter was not present locally, so its dist/style.css wasn't inspected.
- Runtime check: open a dialog and inspect computed styles — confirm .v-modal does NOT render a backdrop/animations and that .tiny-dialog-box__wrapper.is-modal is the only mask.
- If legacy rules still apply, add a docs-only override loaded last. Example (examples/sites/src/assets/overrides/reset-legacy-modal.css):
/* Neutralize legacy overlay/animations from next-remoter for docs site */ .v-modal { background: transparent !important; box-shadow: none !important; } .v-modal, .v-modal-enter-active, .v-modal-leave-active { animation: none !important; transition: none !important; }Import it after other CSS in main.js:
import './assets/overrides/reset-legacy-modal.css'
examples/sites/src/main.js
Outdated
| @@ -1,5 +1,8 @@ | |||
| import { createHead } from '@vueuse/head' | |||
| import { createApp } from 'vue' | |||
|
|
|||
| // 由于 @opentiny/next-remoter/dist/style.css 中包含了旧版的弹窗和动画样式，这里调整到最前，不确定是否会有其他影响，请评估 | |||
| import '@opentiny/next-remoter/dist/style.css' | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm dependency presence and single import.
Ensure @opentiny/next-remoter is declared (root/examples package.json) and not imported elsewhere (e.g., App.vue), to avoid dup loads and 404s in CI.
🏁 Script executed:
#!/bin/bash
# Confirm a single import site
rg -nP "@opentiny/next-remoter/dist/style\\.css"
# Verify dependency is declared (adjust path if examples has its own package.json)
if command -v jq >/dev/null; then
fd -t f package.json -d 2 | xargs -I{} sh -c 'echo "{}:"; jq -r ".dependencies[\"@opentiny/next-remoter\"], .devDependencies[\"@opentiny/next-remoter\"]" "{}"'
else
rg -nP '"@opentiny/next-remoter"\s*:' --glob package.json
fiLength of output: 211
Declare @opentiny/next-remoter in package.json or remove the import
- Single import found: examples/sites/src/main.js:5 — import '@opentiny/next-remoter/dist/style.css'.
- Root package.json does not declare @opentiny/next-remoter (jq returned null). Add the dependency to the appropriate package.json (examples/sites/package.json or repo root) or remove the import to avoid CI 404s and duplicate loads.
🤖 Prompt for AI Agents
In examples/sites/src/main.js at line 5, the file imports
'@opentiny/next-remoter/dist/style.css' but that package is not declared in
package.json; either add @opentiny/next-remoter (appropriate version) to
examples/sites/package.json or the repo root package.json so the dependency is
installed, then run install to update lockfile, or remove the import from
examples/sites/src/main.js if the stylesheet is not needed; ensure package.json
dependency is added to the correct package (examples/sites) to avoid CI 404s and
duplicate loads.
820049f to
4bc7a09
Compare
gausszhou
force-pushed
the
zcg/issue-3636
branch
from
4bc7a09 to
7284629
Compare
gausszhou
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue-hooks/src/vue-popup.ts (1)
159-176: closeDelay bug: reading fromvm.closeDelayignores prop; delays won’t apply.
openDelayreads fromprops.openDelay, butcloseDelayreads fromvm.closeDelay, likely undefined, causing immediate close. Align withprops.closeDelay.Apply this diff:
- const closeDelay = Number(vm.closeDelay) + const props: any = (vm as any).$props || vm + const closeDelay = Number(props.closeDelay)
🧹 Nitpick comments (1)
packages/vue-hooks/src/vue-popup.ts (1)
114-120: Prefer props.modalFade when present; fall back to !props.noAnimationIn packages/vue-hooks/src/vue-popup.ts pass (props.modalFade ?? !props.noAnimation) to PopupManager.openModal to preserve backward compatibility.
PopupManager.openModal( vm._popupId, PopupManager.nextZIndex(), props.modalAppendToBody ? undefined : dom, props.modalClass, - !props.noAnimation + (props.modalFade ?? !props.noAnimation) )
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: #3636
What is the new behavior?
Does this PR introduce a breaking change?
Other information
已通过 e2e 测试
Summary by CodeRabbit
New Features
Behavior Changes