Repository navigation
[DREAM-774] Document sortable lists in Lookbook - #25137
Conversation
myabc
added
documentation
DO NOT MERGE
javascript
There was a problem hiding this comment.
🟡 Changes recommended
The new Lookbook guide includes an ambiguous value key name (move_url_templates_value) that is inconsistent with the documented sortable_lists_* data-hash convention and can mislead consumers into wiring the controller incorrectly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new Lookbook pattern guide for the sortable-lists Stimulus controller suite and documents migration away from the legacy dragula-based generic-drag-and-drop, while also marking the old controllers as deprecated in-code to discourage new usage.
Changes:
- Added a comprehensive Lookbook pattern page describing
sortable-listswiring, request/response contract, accessibility move menus, and migration notes. - Marked
GenericDragAndDropControlleras deprecated with guidance pointing readers tosortable-lists. - Marked existing surfaces inheriting from
GenericDragAndDropControlleras deprecated to prompt future migration.
File summaries
| File | Description |
|---|---|
| lookbook/docs/patterns/40-drag-and-drop.md.erb | New Lookbook documentation for sortable lists (manual wiring + migration guidance). |
| frontend/src/stimulus/controllers/dynamic/generic-drag-and-drop.controller.ts | Adds @deprecated JSDoc to the legacy dragula-based controller. |
| frontend/src/stimulus/controllers/dynamic/meetings/drag-and-drop.controller.ts | Marks meetings drag-and-drop surface as deprecated due to inheritance. |
| frontend/src/stimulus/controllers/dynamic/admin/type-form-configuration/drag-and-drop.controller.ts | Marks type form configuration drag-and-drop surface as deprecated due to inheritance. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2a9e562 to
9d6e696
Compare
myabc
changed the title
9d6e696 to
87304ac
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The review found concrete documentation issues (incorrect base classes in examples and broken markdown formatting) and the PR’s effective scope appears broader than the described intent, needing clarification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/68 changed files
- Comments generated: 1
- Review effort level: Lite
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
| render(Primer::Box.new(display: :flex, align_items: :center)) do | ||
| concat( | ||
| render( |
There was a problem hiding this comment.
@HDinger This should become a PlainCard eventually: https://community.openproject.org/wp/DREAM-750
Adds a Behaviors section holding a tabbed sortable lists page (one list, sibling lists, nested lists, reference) with two hand-wired previews, and turns the drag and drop pattern page into a short design-facing draft. Numbers the top-level page directories so the new section sits between Patterns and Styles; page URLs are unchanged. https://community.openproject.org/wp/DREAM-774
The dragula-based controller is closed to new consumers, but nothing in the source said so. The tag points readers at sortable-lists before they copy a call site.
87304ac to
0aa026b
Compare
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| * **Sortable list, sortable item, sortable item type**: a list whose order the | ||
| server persists, a row that takes part in that order, and the category a | ||
| list accepts. A row is an item once it carries | ||
| `data-sortable-lists--item-id-value`; that is what counts it and anchors | ||
| its neighbors' drops. The `sortable-lists--item` controller on it is what | ||
| makes it draggable and gives it a move menu, and `confined` keeps a | ||
| draggable item in its own list. | ||
| * **Drag source**: the original row while it is being dragged. **Drag | ||
| preview**: the copy that follows the pointer. | ||
| * **Drop indicator**: the line marking where the row will land, drawn without | ||
| reserving space. **Drop container**: the list-level outline shown while a | ||
| compatible row hovers a list (`data-drop-container="active"`). | ||
| * **Reorder** happens within one list; a **move** changes list. In terms of | ||
| the backend request they are identical. |
There was a problem hiding this comment.
Do we want a diagram here too? - or does Behaviours already cover it?
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
HDinger
left a comment
HDinger
left a comment
There was a problem hiding this comment.
I like the images a lot, thanks for spending the effort 🙇
I think the biggest remark for me is the overall structure.. Thinking about how to make it easier for other devs to discover all of that, I think this can be improved. See especially this remark.
| @@ -0,0 +1,188 @@ | |||
| --- | |||
| label: Reference | |||
There was a problem hiding this comment.
I think the name is not really self explaining.. Without reading it, I'd not know what to expect from this file.
There was a problem hiding this comment.
TIL that something like this is possible. Nice 👍
There was a problem hiding this comment.
It's the first time we're using this, but I think it helps make the page more manageable where we have a bunch of different options. Here's the Lookbook docs - just a matter of naming the .md.erb files accordingly.
There was a problem hiding this comment.
I am not a big fan of the naming, both "behaviours" and "sortable-lists" as the name of the page... Why is that separated from the drag-and-drop page you added? Nobody will understand that they have to look below behaviors/sortable-lists if they are looking for how drag&drop can be implemented..
There was a problem hiding this comment.
@HDinger I've tried consolidating everything on the Patterns → Drag and drop page. Let me know what you think?
Consolidates design guidance and sortable-list implementation under Patterns > Drag and drop to address review discoverability feedback. Keeps scenarios in separate tabs, clarifies the API reference label, and fixes scenario spacing and the invisible draft notice. Refs DREAM-774.
HDinger
left a comment
HDinger
left a comment
There was a problem hiding this comment.
Looks really nice. Thanks for all the effort 🙇 👏 🌟
| label: Design guidance | ||
| --- | ||
|
|
||
| > **Draft:** The design guidance on this page has not been reviewed by the design team yet. |
There was a problem hiding this comment.
I think, I would remove that before merging. I guess it is more confusing for people outside our team than it helps. Internally we can still request a review from the design team
Removes draft notice about design guidance review status.
Note
#21688 (OP-18657) and #25139 (DREAM-821) are not dependencies of this PR and as such there are no links to Stimulus API doc/source code.
Note
This documentation will evolve once #24525 is merged.
Ticket
https://community.openproject.org/wp/DREAM-774
What are you trying to accomplish?
Document how to implement drag and drop with the
sortable-listsStimulus suite, alongside the design guidance for the interaction. This is the documentation that DREAM-786 left to this ticket.Patterns → Drag and drop brings the design guidance and implementation documentation together on one Lookbook page, with six tabs:
generic-drag-and-dropand its two subclasses also carry@deprecatedTSDoc pointing atsortable-lists. No call sites are migrated here.Screenshots
N.B. screenshots were made before b6545fe was pushed
Patterns → Drag and drop
Behaviors → Sortable lists
Intro
In depth
What approach did you choose and why?
data-*attributes; the Railsdata:hash form is explained once in the Reference tab. The one exception is the move menu, which depends on Primer'sActionMenuand is therefore shown as itswith_itemcall.Merge checklist
sortable-lists.controller.spec.tsand the reorder feature specs