Add keyboard shortcut (Mod+Z / Mod+Y) for undo-redo in drawing section #472
Conversation
|
Hey! All functionality has been tested locally — both buttons and shortcuts trigger the same undo() and redo() handlers correctly. Looking forward to your review and feedback. |
There was a problem hiding this comment.
Nice work! I have a few comments, some stylistic and others more questions because it looks like this PR has two different implementations of redo/undo (one globally via shortcuts.ts and the editor reducer and one in the component).
I've used https://conventionalcomments.org/ to style my comments :)
app/src/app/components/note-viewer/elements/drawing/DrawingElementComponent.tsx
Show resolved
Hide resolved
app/src/app/components/note-viewer/elements/drawing/DrawingElementComponent.tsx
Show resolved
Hide resolved
| setTimeout(() => this.initCanvas(), 0); | ||
|
|
||
| // setTimeout(() => this.initCanvas(), 0); | ||
| this.initCanvas(); |
There was a problem hiding this comment.
Question: This was intentionally in a 0ms setTimeout to ensure the init canvas happened on the next cycle of the event loop (and thus outside of the context of a componentDidUpdate). It's been many years since I wrote a lot of this code-- can you please ensure that doing the init right here is okay?
| this.ctx.lineTo(pos.x, pos.y); | ||
|
|
||
| this.ongoingTouches.deleteTouch(event.pointerId); | ||
| this.pushUndo(); |
| future: [], | ||
| }, | ||
| reducers: {}, | ||
| extraReducers: builder => builder |
There was a problem hiding this comment.
issue: Reducers must be immutable, so directly mutating state in these cases is not allowed. Please do immutable updates like was done before. If you want to do in-place array manipulation with push/pop please clone the arrays with ... first.
| shouldWordWrap: true, | ||
| drawMode: DrawMode.Line, | ||
| drawingLineColour: '#000000', | ||
| drawings: [], |
There was a problem hiding this comment.
Suggestion (non-blocking): These are all related (as is drawMode, the line colour, etc.) I wonder if it's worth making a drawingState: { /* etc. */ } in here. It's fine as-is though so up to you (although I might suggest prefixing past and future to be like drawingPast and drawingFuture)
| collapseAllExplorer: actionCreator<void>('COLLAPSE_ALL_EXPLORER'), | ||
| openEditor: actionCreator<string>('OPEN_EDITOR'), | ||
| updateElement: actionCreator<UpdateElementAction>('UPDATE_ELEMENT'), | ||
| addDrawing: actionCreator<any>('ADD_DRAWING'), |
There was a problem hiding this comment.
Question: This action doesn't look like it is being dispatched anywhere? Was this part of an earlier approach that has since been replaced by the logic in the component?
|
Hyy hii, my uni mid semester exams are going on, can I connect and resolve the issues on 17 ? |
|
All good, there's no deadline for any of this stuff :) Please reach out if you have any questions or would like a hand. |
2 participants
#468
Summary
This PR adds keyboard shortcut support for Undo (Mod+Z) and Redo (Mod+Y) actions within the Drawing section of MicroPad.
It enhances the user experience by allowing quick undo/redo without using on-screen buttons.
-- Details
Added global keyboard event listeners to detect:
Ctrl + Z / Cmd + Z → triggers Undo
Ctrl + Y / Cmd + Y → triggers Redo
Ensured event listeners are cleaned up on component unmount to prevent memory leaks.
--Testing
Opened the Drawing section and confirmed:
Drawing actions are recorded properly.
Ctrl+Z / Cmd+Z undoes the last stroke.
Ctrl+Y / Cmd+Y redoes the undone stroke.
Tested with multiple consecutive undo/redo operations.
-- Preview
Screen.Recording.2025-10-05.170248.mp4
--Checklist
Code follows project conventions
Undo/Redo logic integrated with existing stack
No version number changes
Feature works as expected