-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
컨텍스트 메뉴를 통한 노드.간선 편집/제거 #163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다
@@ -6,6 +6,7 @@ import { KonvaEventObject } from "konva/lib/Node"; | |||
import { Vector2d } from "konva/lib/types"; | |||
|
|||
type NodeProps = { | |||
id: string; | |||
x: number; | |||
y: number; | |||
draggable?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1점) 이후에 src라는 프로퍼티가 추가될 예정이라서 참고만 해주세요
ContextMenuSeparator, | ||
ContextMenuShortcut, | ||
ContextMenuGroup, | ||
ContextMenuPortal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
양이 굉장하네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지후님 고생많으셨습니다!
shadcn과 konva를 결합하시는 과정이 쉽지 않으셨을텐데, Wrapper를 따로 만드신 것 이것이 저에겐 킥이었습니다.
(1점)
모바일 환경에서도 컨텍스트 메뉴가 잘 작동하는지 확인해봤는데, 크롬 개발자 도구 기준에서는 잘 작동하지 않는 것 같습니다. 참고만 해주세요! (제가 테스트를 잘못했을 수도 있습니다.)
export type ContextMenuItemConfig = { | ||
label: string; | ||
action: () => void; | ||
}; | ||
|
||
export type SelectedNodeInfo = { | ||
id: string; | ||
type: Exclude<Node["type"], "url" | "image" | "head">; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(잡담) 타입을 별도로 분리해주신게 좋은 것 같습니다 ㅎㅎ
다른 분들 코드를 보니 추후 타입 분리에 대한 리팩토링도 필요해보였어요.
const { selectedNode, selectNode, selectedEdge, selectEdge, clearSelection } = | ||
useSpaceSelection(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(0.1점) select("node",...) getSelected(...) 조금 더 추상화된 메서드를 만들어 관리할 수 있겠다는 생각을 했습니다 .. 이건 근데 조금 개인적인 취향입니다
@parkblo 모바일에서의 컨텍스트 메뉴 조금 고민이네요. 생각하지 못했던 부분이에요. 제가 알기로는 onContextMenu(우클릭)이벤트가 모바일에서는 롱클릭으로 대체되는데, |
✏️ 한 줄 설명
우클릭하여 컨텍스트 메뉴의 편집/제거를 통해 노드를 편집하고, 노드.간선을 삭제할 수 있어요.
✅ 작업 내용
🏷️ 관련 이슈
closes #65
closes #64
closes #66
📸 스크린샷/영상
📌 리뷰 진행 시 참고 사항
shadcn의 ContextMenu는 무조건 ContextMenuTrigger를 통해서만 표시돼요. (표시 로직 커스텀이 불가능해요 🥲) 이러한 배경때문에 Stage를 trigger로 하고 우클릭된 target으로 비교했어요.
SpaceView에서 ContextMenu 관련 로직들을 다 구현했다니 복잡해졌어요. 해결을 위해 래핑할 수 있는 컴포넌트로 분리했어요. (ContextMenu 컴포넌트에서는
메뉴 아이템
+클릭 시 실행할 함수
만 주입받을 수 있도록 했어요)props에서 자꾸 객체 형태로 넘겨지는 부분들이 많아지는 것 같아요. 컴포넌트 분리 자체나, 너무 복잡해져서 개선이 필요한 부분이 있으면 알려주세요!