Conversation
Replace the shared Stimulus drag-and-drop controller internals with Atlassian Pragmatic Drag and Drop using explicit container and draggable targets.\n\nUpdate backlogs markup and component specs to use the explicit draggable contract, and add focused frontend coverage for request payloads, registration cleanup, and revert behavior.\n\nVerified with the focused frontend spec and ESLint for touched Stimulus files. Ruby component specs are not runnable in this shell because the required Bundler version from Gemfile.lock is unavailable under the current system Ruby.
myabc
force-pushed
the
spike/pragmatic-drag-and-drop
branch
from
0cb5bfd to
0b408f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates the shared Stimulus generic-drag-and-drop controller from Dragula to Atlassian’s @atlaskit/pragmatic-drag-and-drop and updates Backlogs components/specs to explicitly mark draggable rows via Stimulus targets.
Changes:
- Replace Dragula-based DnD implementation with pragmatic-drag-and-drop (draggable + drop targets + monitor).
- Introduce explicit
data-generic-drag-and-drop-target="draggable"markers for Backlogs rows and assert them in component specs. - Add a new frontend unit spec for the controller and add the new npm dependency.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/app/components/backlogs/sprint_component.rb | Adds explicit draggable target marker to sprint story row data. |
| modules/backlogs/app/components/backlogs/backlog_component.rb | Adds explicit draggable target marker to backlog story row data. |
| modules/backlogs/app/components/backlogs/inbox_item_component.rb | Adds explicit draggable target marker to inbox row data. |
| modules/backlogs/spec/components/backlogs/sprint_component_spec.rb | Asserts new draggable target marker is rendered. |
| modules/backlogs/spec/components/backlogs/backlog_component_spec.rb | Asserts new draggable target marker is rendered. |
| modules/backlogs/spec/components/backlogs/inbox_item_component_spec.rb | Adds assertions for the new draggable target marker and metadata. |
| frontend/src/stimulus/controllers/dynamic/generic-drag-and-drop.controller.ts | Replaces Dragula with pragmatic-drag-and-drop and introduces explicit draggable target registration. |
| frontend/src/stimulus/controllers/dynamic/generic-drag-and-drop.controller.spec.ts | Adds unit coverage for cleanup, data building, and revert-on-failure. |
| frontend/package.json | Adds @atlaskit/pragmatic-drag-and-drop dependency. |
| frontend/package-lock.json | Locks new dependency and transitive packages. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
modules/backlogs/app/components/backlogs/inbox_item_component.rb:70
row_optionsalways marks the row as a generic drag-and-dropdraggableand sets draggable metadata, even whendraggable?is false (the handle is not rendered in that case). With the new pragmatic-drag-and-drop integration, this can make rows draggable without an explicit handle unless the controller blocks it. Consider only adding the draggable data attributes/CSS class/target whendraggable?is true to keep behavior consistent with the UI and other backlogs components.
def row_options
{
id: dom_id(work_package),
classes: "Box-row--hover-blue Box-row--focus-gray Box-row--clickable Box-row--draggable",
data: {
generic_drag_and_drop_target: "draggable",
draggable_id: work_package.id,
draggable_type: "story",
drop_url: move_project_inbox_path(project, work_package),
story: true,
| connect() { | ||
| this.autoscroll?.destroy(); | ||
| this.drake?.destroy(); | ||
| this.initDrake(); | ||
| this.monitorCleanup?.(); | ||
| this.monitorCleanup = monitorForElements({ | ||
| onDrop: (args) => { | ||
| void this.handleMonitorDrop(args as MonitorDropArgs); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
monitorForElements() is registered without any scoping filter. Because monitors are global, every mounted GenericDragAndDropController instance will receive the same drop events and may optimistically move elements and/or issue duplicate PUT requests. Add a guard (e.g., canMonitor / canDrop checks) so only the controller that owns source.element (and/or the destination container) handles the drop.
| export default class GenericDragAndDropController extends Controller { | ||
| static targets = ['container', 'scrollContainer']; | ||
| static targets = [ | ||
| 'container', | ||
| 'scrollContainer', | ||
| 'draggable', | ||
| ]; |
There was a problem hiding this comment.
Adding the draggable Stimulus target makes this controller depend on data-generic-drag-and-drop-target="draggable" being present on every draggable row. There are existing usages that only set data-draggable-id/type/drop-url (e.g., Admin::Enumerations::IndexComponent, WorkPackageTypes::ExportTemplateListComponent), which will no longer register as draggables and will silently break drag-and-drop. Either keep backwards compatibility (auto-register elements with data-draggable-id within connected containers) or update all existing call sites in the same change set.
| const cleanup = combine( | ||
| draggable({ | ||
| element: target, | ||
| dragHandle: target.querySelector(this.handleSelectorValue) ?? undefined, | ||
| canDrag: () => true, | ||
| getInitialData: () => ({ | ||
| draggableType: target.getAttribute('data-draggable-type'), | ||
| }), | ||
| onDragStart: () => { | ||
| this.isDragging = true; | ||
| this.currentDragElement = target; | ||
| this.dragOriginSource = target.parentElement; | ||
| this.dragOriginNextSibling = target.nextElementSibling; | ||
| this.setHandlePressed(target, true); | ||
| }, | ||
| ); | ||
| }); | ||
| onDrop: () => { | ||
| this.isDragging = false; | ||
| this.setHandlePressed(target, false); | ||
| }, | ||
| }), |
There was a problem hiding this comment.
draggable() is configured with canDrag: () => true while dragHandle is optional. If a row intentionally does not render a .DragHandle (e.g., when the user lacks permissions), dragHandle becomes undefined and the whole row can become draggable, changing behavior from the previous Dragula implementation (which only allowed dragging via the handle). Consider requiring a handle to exist (e.g., canDrag returns false when no handle is found) and/or skipping draggable registration when the handle is missing.
|
Closing |
Ticket
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist