Crosshair cursor for colour placement#585
Conversation
📝 WalkthroughWalkthroughRefactors cursor handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/gizmos.js (1)
210-216:⚠️ Potential issue | 🟡 MinorUse shared cursor helpers here (and remove the transient inline comment).
This path manually mutates cursor state instead of using
setCrosshairCursor()/setDefaultCursor(), which makes cursor behavior easier to drift from other flows. Also,// worksis not a durable code comment.♻️ Proposed fix
- // restore cursors - document.body.style.cursor = "default"; - flock.scene.defaultCursor = ""; + setDefaultCursor(); return; @@ - document.body.style.cursor = "crosshair"; // works - flock.scene.defaultCursor = "crosshair"; + setCrosshairCursor();As per coding guidelines
**/*.{js,ts,tsx,jsx,py,java,go,rb,php,cs,cpp,c}: "Comments should be infrequent", "Comments should be genuinely noteworthy", and "Comments should reflect the current state of code only."Also applies to: 223-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/gizmos.js` around lines 210 - 216, Replace the manual cursor mutations and transient comment in the out-of-bounds branch with the shared cursor helpers: instead of setting document.body.style.cursor and flock.scene.defaultCursor directly, call setDefaultCursor() (and use setCrosshairCursor() where appropriate in nearby logic); keep the existing cleanup steps (removeEventListener("click", onPickMesh) and stopCanvasKeyboardMode()) intact and remove the inline "// restore cursors" / "// works" comments. Apply the same replacement for the other similar block that directly mutates cursors (the block that currently sets document.body.style.cursor and flock.scene.defaultCursor around onPickMesh/stopCanvasKeyboardMode).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/gizmos.js`:
- Around line 210-216: Replace the manual cursor mutations and transient comment
in the out-of-bounds branch with the shared cursor helpers: instead of setting
document.body.style.cursor and flock.scene.defaultCursor directly, call
setDefaultCursor() (and use setCrosshairCursor() where appropriate in nearby
logic); keep the existing cleanup steps (removeEventListener("click",
onPickMesh) and stopCanvasKeyboardMode()) intact and remove the inline "//
restore cursors" / "// works" comments. Apply the same replacement for the other
similar block that directly mutates cursors (the block that currently sets
document.body.style.cursor and flock.scene.defaultCursor around
onPickMesh/stopCanvasKeyboardMode).
The cursor should be a crosshair when you are filling with the colour picker. It isn't, but I think this fixes it. I haven't fully tested whether this has any knock on effects.
Summary by CodeRabbit