Added hotkey for deleting nodes #637
Conversation
|
| Name | Link |
|---|---|
| 🔨 Latest commit | befcf24 |
✅ Deploy Preview for ameliorate-docs canceled.
|
| @@ -8,6 +10,20 @@ export const DeleteNodeMenuItem = ({ node }: { node: Node }) => { | |||
| const { sessionUser } = useSessionUser(); | |||
| const userCanEditTopicData = useUserCanEditTopicData(sessionUser?.username); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
This is a good idea, but since this useEffect is defined within the DeleteNodeMenuItem, the delete key only works when this menu item is rendered, i.e. when right-clicking on a node.
Check out the Technical Details section of the issue #520 - the code referenced there shows an established pattern for adding hotkeys in Ameliorate. There, we add a hotkey hook to the TopicWorkspace, which is a parent container of the Diagram, so the hotkey will always be usable without needing any other specific components rendered.
There was a problem hiding this comment.
The code you've added to TopicWorkspace is looking better, but this DeleteNodeMenuItem code still needs to be undone.
sarccasm
force-pushed
the
feature/delete-node-hotkey
branch
from
afe4e92 to
297b016
Compare
| @@ -37,11 +48,19 @@ const useWorkspaceHotkeys = (user: { username: string } | null | undefined) => { | |||
| const [score] = hotkeysEvent.keys; | |||
| if (!score || !possibleScores.some((s) => score === s)) return; | |||
|
|
|||
| // seems slightly awkward that there's logic here not reused with the Score component, but hard to reuse that in a clean way | |||
There was a problem hiding this comment.
Did you intentionally remove this comment? It's still relevant for the code below it, and your hotkey changes seem unrelated to it. The same goes for the other two comments that you've removed.
| const myUsername = isOnPlayground() ? playgroundUsername : user?.username; | ||
| if (!userCanEditScores(myUsername, getPerspectives(), getReadonlyMode())) return; | ||
| setScore(myUsername, selectedPart.id, score as Score); | ||
| }); | ||
|
|
||
| useHotkeys("delete", () => { |
There was a problem hiding this comment.
We should maintain consistency with the other hotkeys above by creating a hotkeys.delete constant for "delete". The purpose for hotkeys.tsx is so that there's one spot to easily see what all the hotkeys are, and that we can update the button equivalent of the hotkey to show the hotkey if you hover it, like this:
| useHotkeys("delete", () => { | ||
| const selectedNodeId = getSelectedNodeId(); | ||
| if (selectedNodeId && nodeExists(selectedNodeId)) { | ||
| deleteNode(selectedNodeId); |
There was a problem hiding this comment.
As mentioned in the issue, this logic should work for both nodes and edges. Instead of deleteNode, we can just use the deleteGraphPart method, the same way the Delete button does:
| const getSelectedNodeId = (): string | null => { | ||
| const selectedPart = getSelectedGraphPart(); | ||
| return selectedPart ? selectedPart.id : null; | ||
| }; | ||
|
|
||
| const nodeExists = (nodeId: string): boolean => { | ||
| const selectedPart = getSelectedGraphPart(); | ||
| return selectedPart ? selectedPart.id === nodeId : false; | ||
| }; |
There was a problem hiding this comment.
I don't think these methods are needed. We can just return from our hook early if the getSelectedGraphPart() is null, like how the hotkey for scoring behaves:
ameliorate/src/web/topic/components/TopicWorkspace/TopicWorkspace.tsx
Lines 35 to 36 in 778fcf2
And if the selected part isn't null, we can pass it directly to the deleteGraphPart method.
| @@ -8,6 +10,20 @@ export const DeleteNodeMenuItem = ({ node }: { node: Node }) => { | |||
| const { sessionUser } = useSessionUser(); | |||
| const userCanEditTopicData = useUserCanEditTopicData(sessionUser?.username); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
The code you've added to TopicWorkspace is looking better, but this DeleteNodeMenuItem code still needs to be undone.
Closes #[520]
Description of changes
Added a keyboard shortcut (Delete) for deleting nodes in the graph editor.
Implemented useEffect with an event listener for keydown in DeleteNodeMenuItem.tsx.
Additional context
Verified that nodes can be deleted using both the context menu and the Delete key.