Make all Studio modals draggable by their header - #3931
Open
vin0401 wants to merge 2 commits into
Open
Conversation
Introduce a shared `useDraggableModal` hook that wraps modal content in react-draggable using the standard antd `modalRender` recipe (header/title drag handle via a CSS selector, viewport-clamped bounds, nodeRef, controlled position reset to centre on each open). Wire it into both modal surfaces: - Declarative: base `Modal` gains a `draggable` prop (default true); a caller-provided `modalRender` still takes precedence. - Imperative: `useStudioModal` wraps confirm/info/success/error/warning so the form and alert modals become draggable, composing with any existing modalRender. Refactor `WindowModal` to consume the shared hook (single drag implementation) and migrate the remaining direct `App.useApp().modal` consumers to `useStudioModal` so their dialogs are draggable too. The cross-iframe registrar (app-loader / modalApi) intentionally keeps the raw instance to avoid double-wrapping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Verdict: Needs changes. The PR centralizes draggable behavior for shared declarative and imperative modals.
Changes:
- Adds a shared draggable-modal hook and imperative renderer.
- Enables dragging by default in the shared
Modal. - Migrates imperative modal consumers to
useStudioModal.
Assessment:
- Root cause/boundary: Correctly targets shared modal infrastructure.
- Call-site coverage: Incomplete; direct Ant Design
Modalconsumers remain non-draggable. - Compatibility: Additive API, with custom
modalRenderpreserved. - Tests/docs: Inline documentation added, but no regression tests or changelog.
- Risk: Iframe bounds use the child viewport despite rendering in the parent document.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
assets/js/src/sdk/components/index.ts |
Exports draggable-modal APIs. |
assets/js/src/core/modules/perspectives/hooks/use-perspectives.tsx |
Migrates local modal access. |
assets/js/src/core/modules/element/hooks/use-element-actions-menu.tsx |
Migrates local modal access. |
assets/js/src/core/modules/document/actions/site/use-site-actions.tsx |
Migrates local modal access. |
assets/js/src/core/modules/document/actions/site/provider/site-modal-provider.tsx |
Migrates local modal access. |
assets/js/src/core/modules/document/actions/paste/use-paste.tsx |
Migrates local modal access. |
assets/js/src/core/modules/document/actions/add-page/use-add-document.tsx |
Migrates local modal access. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/keys-tab.tsx |
Migrates local modal access. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/groups-tab.tsx |
Migrates local modal access. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/collections-tab.tsx |
Migrates local modal access. |
assets/js/src/core/modules/classification-store-config/components/sidebar/sidebar.tsx |
Migrates local modal access. |
assets/js/src/core/components/modal/window-modal/window-modal.tsx |
Reuses the shared drag hook. |
assets/js/src/core/components/modal/modal.tsx |
Enables declarative dragging by default. |
assets/js/src/core/components/modal/hooks/use-studio-modal.tsx |
Wraps imperative modal methods. |
assets/js/src/core/components/modal/hooks/use-draggable-modal.tsx |
Implements shared drag state and bounds. |
assets/js/src/core/components/modal/hooks/use-draggable-modal.styles.tsx |
Adds drag cursor styling. |
assets/js/src/core/components/modal/hooks/draggable-modal-render.tsx |
Composes imperative modal renderers. |
assets/js/src/core/components/modal-upload/hooks/use-upload-conflict-modal.tsx |
Migrates upload-conflict dialogs. |
| }, [open]) | ||
|
|
||
| const onStart = useCallback((_event: DraggableEvent, uiData: DraggableData): void => { | ||
| const { clientWidth, clientHeight } = window.document.documentElement |
| } | ||
|
|
||
| export const Modal = ({ iconName, size = 'M', limitContentHeight, className, title, children, styles: stylesProp, ...props }: IModalProps): React.JSX.Element => { | ||
| export const Modal = ({ iconName, size = 'M', limitContentHeight, className, title, children, styles: stylesProp, draggable = true, modalRender: modalRenderProp, ...props }: IModalProps): React.JSX.Element => { |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Resolves https://github.com/pimcore/product-management/issues/1259
Makes every Studio modal draggable by its header/title, via a single shared drag implementation.
Shared core
useDraggableModalhook — wraps modal content inreact-draggableusing the standard antdmodalRenderrecipe: drag handle scoped to.ant-modal-header/.ant-modal-confirm-titlevia a CSS selector (close button, buttons, links and form controls in the header are excluded from initiating a drag), viewport-clampedboundsrecomputed on drag start,nodeRef(React 18/19 safe), and a controlledpositionthat resets to centre each time the modal opens.DraggableModalRender/withDraggableModalRender— component + helper for the imperative modal API, composing with any caller-providedmodalRender.Two wiring surfaces
Modalgains adraggable?: booleanprop (defaulttrue), so all existingModalusages inherit dragging. A caller-suppliedmodalRenderalways wins and disables the built-in behaviour. Mask / centering / focus-trap are untouched.useStudioModalwrapsconfirm/info/success/error/warning, souseFormModalanduseAlertModaldialogs become draggable too.Consolidation
WindowModalnow consumes the shared hook instead of its own copy of the drag logic (keeps itsmask={false}+ iframegetPopupContainerbehaviour).App.useApp().modalconsumers migrated touseStudioModalso their dialogs are draggable as well.app-loader/modalApi) intentionally keeps the raw modal instance — child iframes re-wrap it viauseStudioModal, so wrapping there would nest twoDraggables.🤖 Generated with Claude Code