feature: Add Drag and Drop support to uni-dom #405
Conversation
Add DragDrop object for building drag-and-drop UIs: - DragData/DragState for type-safe data transfer and state tracking - draggable() to make elements draggable with data - dropZone() for receiving drops with optional type filtering - fileDropZone() for handling file drops from the OS - Reactive state tracking with isDragging and state streams - Event handlers for custom drag behavior Also adds: - DomNode.group() for combining multiple DomNodes - DomNodeGroup support in DomRenderer Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
doc
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces drag and drop functionality to uni-dom, along with a DomNode.group helper. The implementation is well-structured, using a global reactive state for drag operations. I've found a potential bug in the fileDropZone implementation where the drag state is not correctly reset on drop. I've also noted that the DragDrop.stop() method is currently a no-op, which could be misleading. Additionally, the design document in plans/ is out of sync with the final implementation in a few places. My detailed comments provide suggestions to address these points.
| ondropH { (e: dom.DragEvent) => | ||
| e.preventDefault() | ||
| instance.setOverElement(None) | ||
|
|
||
| val files = e.dataTransfer.files | ||
| if files.length > 0 then | ||
| val fileSeq = (0 until files.length).map(files(_)) | ||
| onFiles(fileSeq) | ||
| } |
There was a problem hiding this comment.
The ondrop handler for fileDropZone is missing a call to instance.endDrag(). This is inconsistent with dropZone and can lead to the isDragging state remaining true after an in-app draggable element is dropped on a file drop zone. This can cause UI inconsistencies, such as a drag overlay not disappearing. You should call instance.endDrag() to ensure the drag state is reset upon drop, for both consistency and correctness.
ondropH { (e: dom.DragEvent) =>
e.preventDefault()
instance.setOverElement(None)
val files = e.dataTransfer.files
if files.length > 0 then
val fileSeq = (0 until files.length).map(files(_))
onFiles(fileSeq)
instance.endDrag()
}| ### Core Types | ||
|
|
||
| ```scala | ||
| // Data being transferred during drag | ||
| case class DragData( | ||
| kind: String, // Type identifier (e.g., "item", "file", "text") | ||
| data: Any, // The actual data | ||
| effectAllowed: String = "all" // copy, move, link, all, etc. | ||
| ) | ||
|
|
||
| // Current state of drag operation | ||
| case class DragState( | ||
| isDragging: Boolean, | ||
| data: Option[DragData], | ||
| overElement: Option[dom.Element] | ||
| ) | ||
| ``` | ||
|
|
||
| ### DragDrop Object | ||
|
|
||
| ```scala | ||
| object DragDrop: | ||
| // Make an element draggable | ||
| def draggable(data: DragData): DomNode | ||
| def draggable(kind: String, data: Any): DomNode | ||
|
|
||
| // Create a drop zone | ||
| def dropZone(onDrop: DragData => Unit): DomNode | ||
| def dropZone(accept: String*)(onDrop: DragData => Unit): DomNode | ||
|
|
||
| // File drop zone (for files from OS) | ||
| def fileDropZone(onFiles: Seq[dom.File] => Unit): DomNode | ||
|
|
||
| // Reactive state | ||
| def state: Rx[DragState] | ||
| def isDragging: Rx[Boolean] | ||
|
|
||
| // Event handlers for custom behavior | ||
| def onDragStart(handler: DragData => Unit): DomNode | ||
| def onDragEnd(handler: () => Unit): DomNode | ||
| def onDragOver(handler: dom.DragEvent => Unit): DomNode | ||
| def onDragEnter(handler: dom.DragEvent => Unit): DomNode | ||
| def onDragLeave(handler: dom.DragEvent => Unit): DomNode | ||
| ``` |
There was a problem hiding this comment.
The API design described in this document has several inconsistencies with the actual implementation in DragDrop.scala. It's important to keep design documents up-to-date to avoid confusion for future developers.
Specifically:
DragData.datais defined with typeAny, but implemented asString.draggable(kind: String, data: Any)is defined, but implemented asdraggable(kind: String, data: String).onDragStarthandler is defined asDragData => Unit, but implemented asdom.DragEvent => Unit.onDragEndhandler is defined as() => Unit, but implemented asdom.DragEvent => Unit.
Please update this design document to reflect the implemented API.
|
|
||
| def endDrag(): Unit = stateVar := DragState.empty | ||
|
|
||
| override def cancel: Unit = () |
There was a problem hiding this comment.
The cancel method is currently a no-op, which means DragDrop.stop() does nothing. This is misleading given its name and documentation, which states it should 'Stop tracking drag state'. If the intention of stop() is to reset the drag state, cancel should be implemented to do so. This would also be useful for cleaning up in component unmount lifecycle hooks.
override def cancel: Unit = stateVar := DragState.empty- Remove hardcoded dropEffect="move" to allow browser to determine appropriate effect based on effectAllowed - Track drag state for external file drags in fileDropZone Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make cancel method reset drag state instead of no-op - Update plan document to match actual implementation - DragData.data is String, not Any - Event handlers take dom.DragEvent, not DragData - Add missing API methods to plan Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
DragDropobject for building drag-and-drop UIsDomNode.group()for combining multiple DomNodes as a single unitDomNodeGroupsupport in DomRendererFeatures
Drag and Drop API
DomNode.group
API Summary
DragDrop.draggable(data)DragDrop.dropZone(onDrop)DragDrop.dropZone(kinds...)(onDrop)DragDrop.fileDropZone(onFiles)DragDrop.stateDragDrop.isDraggingDomNode.group(nodes...)Test plan
🤖 Generated with Claude Code