improved functionality making modals draggable and cleanup after closing - #1071
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates modal dragging to operate on the specific modal instance and cleans up its draggable widget and namespaced event listener on close, while scoping position, style, and pointer-event resets to that modal. Sequence diagram for modal drag setup and cleanupsequenceDiagram
participant User
participant Modal
participant Dialog as ModalDialog
participant Header as ModalHeader
User->>Modal: show.bs.modal
Modal->>Dialog: draggable({handle: '.modal-header'})
User->>Header: mousedown
Header->>Modal: css(pointerEvents: none)
Header->>Modal: css(modal-content pointerEvents: all)
User->>Modal: hidden.bs.modal
Modal->>Dialog: draggable('destroy')
Modal->>Header: off('mousedown.modalDrag')
Modal->>Dialog: css(left: 0px, top: 0px)
Modal->>Modal: css(pointerEvents: auto)
Modal->>Modal: find('.modal-content').css(pointerEvents: auto)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Eagle.ts" line_range="5752" />
<code_context>
+ modal.find('.modal-header').off('mousedown.modalDrag');
+
+ //reset modal dialog position and styles
+ dialog.css({"left":"0px", "top":"0px"})
+ modal.find("#editFieldModal textarea").attr('style','')
+ modal.find("#issuesDisplayAccordion").parent().parent().attr('style','')
</code_context>
<issue_to_address>
**issue (broader_impact):** The existing `$('.modal').on('hidden.bs.modal', ...)` handler in `src/main.ts` still resets `$('.modal-dialog')` globally whenever any modal closes, so closing one modal resets the position of every modal dialog instead of only the modal being closed.
**Triggers:** When more than one modal is present or another modal remains open while a modal is hidden.
**Suggested fix:** Remove the duplicate global hidden-modal reset in `src/main.ts` or scope that handler to its `this` modal as well.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: src/Eagle.ts:5752
james-strauss-uwa
left a comment
There was a problem hiding this comment.
I just had one question about this. If you need both listeners, and they are different for a good reason, then go ahead and merge.
| @@ -198,7 +198,7 @@ $(function(){ | |||
| } | |||
|
|
|||
| $('.modal').on('hidden.bs.modal', function () { | |||
There was a problem hiding this comment.
Are both this event listener and the event listener in Eagle.ts:5743 required? They both seem to listen for 'hidden.bs.modal'.
And they are slightly different. Maybe add the "const modal" and "const dialog" lines here so they look more similar? The function above does about 8 things, and the function here does 3 of the same things but not the other 5?
It just looks a bit strange.
There was a problem hiding this comment.
good catch! ive removed the main.ts listener function. in fact the eagle one already does everything the one in main.ts did anyways
the code making modals draggable after opening is now targeting that specific modal instead of applying the drag ability status to all.
after closing the modal, the drag listener is cleaned up
Summary by Sourcery
Scope modal dragging and teardown to individual modal instances and restore their state after closing.
Bug Fixes:
Enhancements:
Chores: