fix: centered 为 true 时设置 draggable 无法拖拽 #6948
Conversation
Elm1992
requested review from
anncwb,
jinmao88,
mynetfan and
vince292007
as code owners
|
WalkthroughModified modal component and draggable utility to respect a centered state during drag operations. Transform calculations now conditionally apply centered translation (using Changes
Sequence Diagram(s)sequenceDiagram
participant Modal as modal.vue
participant Composable as useModalDraggable
Note over Modal,Composable: Initial Setup
Modal->>Modal: Compute shouldCentered<br/>(centered && !fullscreen)
Modal->>Composable: Pass centered ref
Note over Modal,Composable: On Drag
Composable->>Composable: Check centered.value
alt Centered is true
Composable->>Composable: Apply centered math<br/>translate(x, calc(-50% + y))
else Centered is false
Composable->>Composable: Apply direct translation<br/>translate(x, y)
end
Composable->>Modal: Update transform
Note over Modal,Composable: On Reset
Composable->>Composable: Clear transform<br/>(set to empty string)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (2)
packages/@core/ui-kit/popup-ui/src/modal/use-modal-draggable.ts (1)
17-18: Center-aware transform logic looks correct; consider de‑duplicating the transform expressionThe new optional
centeredflag and the branch between:
translate(${moveX}px, calc(-50% + ${moveY}px))when centeredtranslate(${moveX}px, ${moveY}px)otherwiseare consistent with the goal of preserving the
-50%baseline while still applying the drag offset, and remain backward compatible for call sites that don’t passcentered.However, the same transform shape is now also constructed in
modal.vuewhen reopening the dialog, which means any future tweak (e.g., adding scale, adjusting axis) must be changed in two places. Consider extracting a small helper (even just a local function in this module that modal.vue can reuse) to build the transform string from(x, y, isCentered)to keep the behavior in one place.Also applies to: 76-82
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (1)
107-109: Centering + draggable wiring is coherent; watch for runtimecenteredtoggles and minor duplicationThe introduction of:
shouldCentered = computed(() => centered.value && !shouldFullscreen.value)- passing
shouldCenteredintouseModalDraggable- and using the same centered vs non-centered transform when reopening:
dialogRef.value.style.transform = shouldCentered.value ? `translate(${offsetX}px, calc(-50% + ${offsetY}px))` : `translate(${offsetX}px, ${offsetY}px)`;lines up well with the hook changes and should fix the “centered + draggable” issue.
A couple of follow‑ups you may want to consider:
Runtime
centeredchanges while open
Thewatchis keyed only onstate?.value?.isOpen, so ifcenteredis toggled while the modal is already open, the classes (top-1/2vs not) will update, but the inlinestyle.transformwon’t be recomputed. Ifcenteredis intended to be a static configuration per modal, this is fine; if it’s expected to be toggled at runtime, you may want a smallwatchonshouldCentered(or awatchEffectthat also depends on it) to recompute the transform or callresetPosition.Avoiding small duplications
- The condition
'top-1/2': centered && !shouldFullscreenis logically equivalent toshouldCentered, so you could referenceshouldCentereddirectly to keep the condition centralized.- The centered/non-centered transform expression is now duplicated here and in
use-modal-draggable.ts; a tiny shared helper (e.g.,getModalTransform(offsetX, offsetY, isCentered)) would keep these in sync.These are refinements; core behavior looks sound.
Also applies to: 117-123, 140-143, 245-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@core/ui-kit/popup-ui/src/modal/modal.vue(5 hunks)packages/@core/ui-kit/popup-ui/src/modal/use-modal-draggable.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-15T04:29:13.944Z
Learnt from: mynetfan
Repo: vbenjs/vue-vben-admin PR: 5397
File: playground/src/bootstrap.ts:23-30
Timestamp: 2025-01-15T04:29:13.944Z
Learning: In the Vue-Vben-Admin project, commented code in bootstrap.ts showing modal and drawer configuration examples should be preserved as reference documentation, even after moving to CSS variables for z-index management.
Applied to files:
packages/@core/ui-kit/popup-ui/src/modal/modal.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). (5)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (2)
packages/@core/ui-kit/popup-ui/src/modal/use-modal-draggable.ts (1)
109-116: Resetting transform with empty string is appropriate hereChanging
resetPositionto settarget.style.transform = ''(instead of forcing'none') resets the inline transform while allowing any CSS class–driven transform (e.g., centering from the baseDialogContentstyles) to take effect again. This aligns with the centered/drag behavior introduced in this PR and avoids clobbering the component’s default transform.No further changes needed in this block.
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (1)
245-247: Styling tweaks for border and fullscreen button look safe
- Changing
'border-border border'to'border border-border'is effectively cosmetic for Tailwind and shouldn’t alter behavior.- The updated
VbenIconButtonclasses (more explicit text color, hover background/text, opacity transition, and focus outline handling) are consistent with the rest of the UI kit and improve visual/interaction feedback without affecting logic.No issues spotted here.
Also applies to: 321-326
2 participants
Description
modal 组件 centered为true时设置draggable无法拖拽
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit