Improve MindElixir UX on mobile devices #364
Conversation
Summary of ChangesHello @meowvelous-elixia, 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 focuses on enhancing the MindElixir user experience, particularly for mobile devices. It introduces intuitive touch gestures like pinch-to-zoom and long-press for dragging, moving away from traditional drag-and-drop events to a more modern PointerEvent API. These changes aim to provide a smoother, more responsive, and consistent interaction model across different platforms, alongside visual adjustments to support these new behaviors. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 refactors node dragging functionality from native HTML Drag and Drop to a custom Pointer Event-based implementation, enhancing touch support and overall interaction. Key changes include introducing multi-touch pinch-to-zoom, long-press activated node dragging for touch devices, and a dedicated nodeDraggable.ts module to manage node drag state and logic. The src/mouse.ts file was heavily modified to integrate these new pointer event handlers, including handlePointerDown, handlePointerMove, handlePointerUp, handlePointerCancel, and handleBlur, while src/index.less received updates for the drag ghost element's styling and iOS user-select behavior. Additionally, src/utils/dom.ts disables native draggable for topics, and src/utils/index.ts gains a getDistance utility. A review comment suggests further encapsulating the drag ghost positioning and display logic within nodeDraggable.ts by modifying showDragGhost to accept an optional initial position, improving separation of concerns.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the event handling logic to improve user experience on mobile devices. By replacing the native HTML Drag and Drop API with Pointer Events, it adds support for common touch gestures like pinch-to-zoom and long-press-to-drag nodes. The code is well-structured, with logic broken down into new helper functions and modules, particularly in mouse.ts and nodeDraggable.ts. The CSS changes also contribute to a better mobile experience by increasing tap target sizes. I have one suggestion to fix a potential bug related to click events firing after a drag operation, but overall this is an excellent improvement.
| export function handleNodeDragEnd(mind: MindElixirInstance, state: NodeDragState, e: PointerEvent): void { | ||
| const { dragged } = mind | ||
| if (!dragged || state.pointerId !== e.pointerId) return | ||
|
|
||
| state.edgeMoveController.stop() | ||
|
|
||
| // Restore opacity | ||
| for (const node of dragged) { | ||
| node.parentElement.parentElement.style.opacity = '1' | ||
| } | ||
|
|
||
| // Hide ghost | ||
| state.ghost.style.display = 'none' | ||
| state.ghost.innerHTML = '' | ||
|
|
||
| // Perform move operation if dragging occurred | ||
| if (state.isDragging && state.meet) { | ||
| clearPreview(state.meet) | ||
| if (state.insertType === 'before') { | ||
| mind.moveNodeBefore(dragged, state.meet) | ||
| } else if (state.insertType === 'after') { | ||
| mind.moveNodeAfter(dragged, state.meet) | ||
| } else if (state.insertType === 'in') { | ||
| mind.moveNodeIn(dragged, state.meet) | ||
| } | ||
| if (meet) insertPreview(meet, insertTpye) | ||
| } | ||
| const off = on([ | ||
| { dom: mind.map, evt: 'dragstart', func: handleDragStart }, | ||
| { dom: mind.map, evt: 'dragend', func: handleDragEnd }, | ||
| { dom: mind.map, evt: 'dragover', func: handleDragOver }, | ||
| ]) | ||
|
|
||
| return off | ||
| // Reset state | ||
| mind.dragged = null | ||
| state.isDragging = false | ||
| state.insertType = null | ||
| state.meet = null | ||
| state.pointerId = null | ||
| } |
There was a problem hiding this comment.
The current implementation resets state.isDragging to false synchronously at the end of a drag. This can lead to an unintended click event being processed immediately after a node is dropped, as the check for isDragging in handleClick (in mouse.ts) will evaluate to false.
To ensure the click is correctly suppressed as intended, you can defer resetting the isDragging flag using setTimeout(..., 0). This keeps the flag as true through the click event, which fires after pointerup, and then resets it.
export function handleNodeDragEnd(mind: MindElixirInstance, state: NodeDragState, e: PointerEvent): void {
const { dragged } = mind
if (!dragged || state.pointerId !== e.pointerId) return
state.edgeMoveController.stop()
// Restore opacity
for (const node of dragged) {
node.parentElement.parentElement.style.opacity = '1'
}
// Hide ghost
state.ghost.style.display = 'none'
state.ghost.innerHTML = ''
const wasDragging = state.isDragging
// Perform move operation if dragging occurred
if (wasDragging && state.meet) {
clearPreview(state.meet)
if (state.insertType === 'before') {
mind.moveNodeBefore(dragged, state.meet)
} else if (state.insertType === 'after') {
mind.moveNodeAfter(dragged, state.meet)
} else if (state.insertType === 'in') {
mind.moveNodeIn(dragged, state.meet)
}
}
// Reset state
mind.dragged = null
if (wasDragging) {
// Defer resetting isDragging to prevent click event after drag
setTimeout(() => {
state.isDragging = false
}, 0)
} else {
state.isDragging = false
}
state.insertType = null
state.meet = null
state.pointerId = null
}
|
@gemini-code-assist review again |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the user experience on mobile devices by replacing the native drag-and-drop functionality with a custom implementation based on Pointer Events. This enables touch-friendly interactions like long-press-to-drag and pinch-to-zoom. The code has been well-refactored, making the event handling logic more modular and robust. I've identified one potential issue regarding the interaction between double-tap and long-press gestures, which I've detailed in a comment. Overall, this is a great enhancement.
| { dom: container, evt: 'pointerup', func: handlePointerUp }, | ||
| { dom: container, evt: 'pointerup', func: handleTouchDblClick }, | ||
| { dom: container, evt: 'pointercancel', func: handlePointerCancel }, | ||
| { dom: container, evt: 'pointerdown', func: handleTouchDblClick }, |
There was a problem hiding this comment.
By changing this event from pointerup to pointerdown, handleTouchDblClick now executes in the same event cycle as handlePointerDown. This introduces a potential issue: when a double-tap is detected, handlePointerDown will have already started a long-press timer for the second tap. This timer is not cleared, which can lead to an unintended drag operation starting after the double-tap has been processed.
To fix this, you should cancel the pending long-press timer within handleTouchDblClick when a double-tap is confirmed.
Suggested change in handleTouchDblClick:
if (tapLength < 300 && tapLength > 0) {
handleDblClick(e)
clearLongPress() // Add this line to cancel the timer
}
2 participants
No description provided.