Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the Backlogs master backlog page’s drag-and-drop implementation with a backlogs-specific Stimulus + Pragmatic Drag and Drop setup that better matches the Hotwire/Turbo morph DOM lifecycle.
Changes:
- Introduces
backlogs--dnd-board,backlogs--dnd-list, andbacklogs--dnd-itemStimulus controllers backed by@atlaskit/pragmatic-drag-and-drop. - Updates Backlogs inbox/sprint markup and Ruby components to expose explicit
ul/lidrop surfaces and per-item metadata (instead of the generic DnD controller + drag handle). - Updates component specs and adds frontend unit specs for the new controllers; adjusts styling to remove the dedicated drag-handle column.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/app/views/rb_master_backlogs/_backlog_list.html.erb | Swaps generic DnD controller for the new backlogs DnD board controller and outlets. |
| modules/backlogs/app/components/backlogs/sprint_component.rb | Adds explicit list args + per-row DnD metadata and controller composition. |
| modules/backlogs/app/components/backlogs/sprint_component.html.erb | Renders sprint content via Backlogs::ListBoxComponent to get an explicit ul/li structure. |
| modules/backlogs/app/components/backlogs/inbox_component.rb | Adds explicit inbox ul list arguments for backlogs--dnd-list. |
| modules/backlogs/app/components/backlogs/inbox_component.html.erb | Switches to Backlogs::ListBoxComponent and ensures the “show more” row provides a DnD prev-id marker. |
| modules/backlogs/app/components/backlogs/inbox_item_component.rb | Adds conditional backlogs--dnd-item controller + metadata on draggable rows. |
| modules/backlogs/app/components/backlogs/inbox_item_component.html.erb | Removes the dedicated drag handle area from inbox items. |
| modules/backlogs/app/components/backlogs/story_component.rb | Removes drag-handle permission logic (and related state). |
| modules/backlogs/app/components/backlogs/story_component.html.erb | Removes drag handle rendering from story cards. |
| modules/backlogs/app/components/backlogs/list_box_component.rb | Adds a new Box-like component that renders ul/li rows for backlogs lists. |
| modules/backlogs/app/components/backlogs/list_box_component.html.erb | Implements the list box markup used by inbox/sprint components. |
| modules/backlogs/spec/components/backlogs/story_component_spec.rb | Updates expectations to reflect removal of a separate drag handle. |
| modules/backlogs/spec/components/backlogs/sprint_component_spec.rb | Updates expectations for list-level and item-level DnD metadata/controllers. |
| modules/backlogs/spec/components/backlogs/inbox_item_component_spec.rb | Updates expectations for row controllers and pragmatic DnD metadata. |
| modules/backlogs/spec/components/backlogs/inbox_component_spec.rb | Adds coverage for the explicit inbox DnD list and “show more” prev-id marker. |
| frontend/src/stimulus/setup.ts | Registers the new backlogs DnD Stimulus controllers. |
| frontend/src/stimulus/controllers/dynamic/backlogs/pragmatic-dnd.ts | Adds a small wrapper module for Pragmatic DnD imports. |
| frontend/src/stimulus/controllers/dynamic/backlogs/dnd-list.controller.ts | Adds list drop target registration + Turbo morph re-registration. |
| frontend/src/stimulus/controllers/dynamic/backlogs/dnd-list.controller.spec.ts | Adds unit tests for list registration and Turbo morph refresh. |
| frontend/src/stimulus/controllers/dynamic/backlogs/dnd-item.controller.ts | Adds draggable + item drop target registration + edge detection + Turbo morph re-registration. |
| frontend/src/stimulus/controllers/dynamic/backlogs/dnd-item.controller.spec.ts | Adds unit tests for item registration, edge detection, and Turbo morph refresh. |
| frontend/src/stimulus/controllers/dynamic/backlogs/dnd-board.controller.ts | Adds board-level monitor that performs optimistic DOM moves and persists via FetchRequest. |
| frontend/src/stimulus/controllers/dynamic/backlogs/dnd-board.controller.spec.ts | Adds unit tests for reorder/move behavior and request payloads. |
| frontend/src/global_styles/primer/_overrides.sass | Adjusts cursor behavior for clickable/draggable Box rows. |
| frontend/src/assets/sass/backlogs/_master_backlog.sass | Removes the drag-handle grid column from backlog card layout. |
| frontend/package.json | Adds @atlaskit/pragmatic-drag-and-drop dependency. |
| frontend/package-lock.json | Locks Pragmatic DnD and its transitive dependencies. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| static values = { | ||
| listId: String, | ||
| accepts: { type: String, default: 'story' }, | ||
| }; | ||
|
|
||
| declare readonly listIdValue:string; | ||
| declare readonly acceptsValue:string; | ||
|
|
||
| private cleanup:(() => void)|null = null; | ||
| private abortController:AbortController|null = null; | ||
|
|
||
| connect() { | ||
| this.abortController?.abort(); | ||
| this.abortController = new AbortController(); | ||
| this.element.addEventListener('turbo:morph-element', this.refreshRegistration, { signal: this.abortController.signal }); | ||
| this.registerPragmaticDnd(); | ||
| } | ||
|
|
||
| disconnect() { | ||
| this.cleanup?.(); | ||
| this.cleanup = null; | ||
| this.abortController?.abort(); | ||
| this.abortController = null; | ||
| } | ||
|
|
||
| private registerPragmaticDnd() { | ||
| this.cleanup?.(); | ||
| this.cleanup = pragmaticDnd.dropTargetForElements({ | ||
| element: this.element, | ||
| canDrop: () => true, | ||
| getData: () => ({ | ||
| kind: 'list', | ||
| listId: this.listIdValue, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
BacklogsDndListController declares an accepts value but never uses it: canDrop is currently unconditional (() => true). That makes the list accept any draggable type and leaves acceptsValue misleading/unvalidated. Consider using acceptsValue inside canDrop (and/or getData) to reject sources whose itemType doesn’t match, so future non-story draggables can’t be dropped into these lists by accident.
|
Closing in favour of #22838 |
f57a777 to
8767457
Compare
Switch item edge detection to Atlassian's hitbox helpers, tighten board filtering and click suppression around drag intent, and update the backlog feature helpers for whole-row dragging. Frontend specs pass, but the Cuprite drag harness for backlog feature specs still needs debugging before those browser specs will go green.
Ticket
https://community.openproject.org/wp/73473
What are you trying to accomplish?
Replace the backlog page drag and drop implementation with a Pragmatic
Drag and Drop based Stimulus solution that fits the current Hotwire
markup better. The active backlog page now owns explicit
ul/lidrop surfaces for inbox and sprint lists, supports reordering and
cross-list moves, and keeps working after Turbo morph refreshes.
What approach did you choose and why?
I avoided extending the existing generic drag-and-drop controller and
built a backlogs-specific spike around three narrow Stimulus
controllers: a board monitor, list drop targets, and item draggable /
drop targets. That let the design follow the backlog markup directly
instead of relying on selector indirection and container accessors.
Turbo morph was preserving list items while stripping the native
draggablestate that Pragmatic DnD had attached. To address that, theitem and list controllers now refresh their Pragmatic registrations on
turbo:morph-element. The row is also draggable as a whole now, whichremoves the separate handle path and simplifies the interaction model.
Merge checklist