Enable cursor support - #225
Conversation
There was a problem hiding this comment.
Pull request overview
Adds UI-level cursor support to the canvas component so the core can drive cursor changes (aimed at improving CAD-style interaction per Issue #10).
Changes:
- Register a cursor-change callback with
core.canvason mount and whencorechanges. - Add
onCursorChangeto map cursor “states” to CSS cursor values. - Unregister the cursor callback on unmount.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -29,13 +32,25 @@ export default class Canvas extends Component{ | |||
| componentDidUpdate(prevProps) { | |||
| if (prevProps.core !== this.props.core) { | |||
There was a problem hiding this comment.
When core changes, the previous prevProps.core.canvas still retains the old cursor callback. That can keep this component alive and can also trigger onCursorChange after the canvas is no longer the active core (or after unmount), leading to errors. Clear the cursor callback on prevProps.core.canvas before registering the new one (mirroring cleanup done in componentWillUnmount).
| if (prevProps.core !== this.props.core) { | |
| if (prevProps.core !== this.props.core) { | |
| prevProps.core.canvas.setCursorCallbackFunction(undefined); |
| // set the cursor callback | ||
| this.props.core.canvas.setCursorCallbackFunction(this.onCursorChange.bind(this)) | ||
|
|
There was a problem hiding this comment.
this.onCursorChange.bind(this) creates a new function each time componentDidMount/componentDidUpdate runs. Consider binding once (e.g., this.boundOnCursorChange = this.onCursorChange.bind(this) in the constructor) and reusing that reference when registering/unregistering the callback; this avoids repeated allocations and makes cleanup reliable if the core API expects the same function reference.
| GRABBING: 'grabbing', | ||
| SELECTION: 'cell', | ||
| }; | ||
| this.canvasRef.current.style.cursor = cursors[state] ?? 'crosshair'; |
There was a problem hiding this comment.
onCursorChange assumes this.canvasRef.current is always available. If a cursor update arrives after unmount (or while swapping cores), this will throw. Add a quick guard (e.g., return early when the ref is missing) to make the callback resilient.
| this.canvasRef.current.style.cursor = cursors[state] ?? 'crosshair'; | |
| const canvas = this.canvasRef.current; | |
| if (!canvas) { | |
| return; | |
| } | |
| canvas.style.cursor = cursors[state] ?? 'crosshair'; |
Complementary PR dubstar-04/Design-Core#260
fixes #10