Skip to content
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

Node 이동 구현 #162

Merged
merged 12 commits into from
Nov 28, 2024
Merged

Node 이동 구현 #162

merged 12 commits into from
Nov 28, 2024

Conversation

parkblo
Copy link
Collaborator

@parkblo parkblo commented Nov 27, 2024

✏️ 한 줄 설명

이 PR의 주요 변경 사항이나 구현된 내용을 간략히 설명해 주세요.

노드를 길게 누르면 이동할 수 있는 기능을 구현했어요.

✅ 작업 내용

  • refactor: 노드 핸들러 타입, SpaceView 렌더링 로직 분리작성
  • feat: 노드 이동

🏷️ 관련 이슈

📸 스크린샷/영상

이번 PR에서 변경되거나 추가된 뷰가 있는 경우 이미지나 동작 영상을 첨부해 주세요.

aasd

📌 리뷰 진행 시 참고 사항

리뷰 코멘트 작성 시 특정 사실에 대해 짚는 것이 아니라 코드에 대한 의견을 제안할 경우, 강도를 함께 제시해주세요! (1점: 가볍게 참고해봐도 좋을듯 ↔ 5점: 꼭 바꾸는 게 좋을 것 같음!)

  • 기존 2초간 홀드가 너무 길다고 느껴져서 0.5초 홀드로 구현하였습니다.
  • 기존에 존재하던 useDragNode와 별도로 사용 가능한 느낌의 커스텀훅을 만들고 싶었는데 현재는 다소 연관이 되어있는 상태입니다.
  • 이벤트 핸들러들을 useCallback으로 묶지 않았는데, 현 상황에서 묶는 방향이 성능에 더 도움이 되는지 궁금합니다 !

@parkblo parkblo added this to the Node 이동 milestone Nov 27, 2024
@parkblo parkblo self-assigned this Nov 27, 2024
@parkblo parkblo requested a review from a team as a code owner November 27, 2024 11:13
@parkblo parkblo requested review from hoqn and heegenie and removed request for a team November 27, 2024 11:13
Copy link
Collaborator

@hoqn hoqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정말 수고 많으셨습니다..! 여러모로 고민한 부분이 많이 보이는 것 같아요.

아래 남긴 코멘트는 그냥 단순하게 남긴 거라 중요한 것은 없습니다

Comment on lines 187 to 192
? GooeyNodeMovingRenderer
: GooeyNodeCreatingRenderer}
{NearIndicatorRenderer}
{NodesRenderer}
{EdgesRenderer}
{PaletteRenderer}
Copy link
Collaborator

@hoqn hoqn Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(0.001점) 이건 개인 취향일 수도 있지만, 저는 컴포넌트만 대문자로, 노드는 일반 변수처럼 카멜케이스를 선호합니다. 이 부분을 누가 맞다고 하기보다도 한 방향으로 맞추는 것도 좋을 것 같아요


const setHoldingAnimation = (
e: KonvaEventObject<MouseEvent> | KonvaEventObject<TouchEvent> | null,
bool: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 변수 이름이 더 명확해도 좋을 것 같아요!

Copy link
Collaborator

@heegenie heegenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꽤 구현 난이도가 있었을 걸로 예상되는데 잘 구현해주신 것 같습니다..!
코드 리뷰할 게 많지 않아 가독성, 재사용성 위주로만 체크했습니다 ㅎㅎ

고생하셨습니다 🐝

packages/frontend/src/hooks/useMoveNode.ts Outdated Show resolved Hide resolved
packages/frontend/src/hooks/useMoveNode.ts Show resolved Hide resolved
@parkblo
Copy link
Collaborator Author

parkblo commented Nov 27, 2024

@hoqn @heegenie
리뷰 감사합니다! 리뷰주신 내용 반영하여 리팩토링 진행했습니다 😄

@parkblo parkblo merged commit d05bb82 into dev Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants