-
Notifications
You must be signed in to change notification settings - Fork 2
[REFACTOR] task click logic update #439
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
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.
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (3)
src/components/targetArea/TargetTaskSection.tsx:10
- [nitpick] The import name 'useTodoSelectionStore' is inconsistent with its usage elsewhere. Consider renaming it to 'useTaskSelectionStore' for consistency.
import useTodoSelectionStore from '@/store/useTaskSelectionStore';
src/components/common/fullCalendar/FullCalendarBox.tsx:38
- [nitpick] Inconsistent naming: consider using 'useTaskSelectionStore' instead of 'useTodoSelectionStore' to maintain consistency across the codebase.
const { selectedTask, clearSelectedTask, setIsDragging } = useTodoSelectionStore();
src/components/common/StagingArea/StagingAreaTaskContainer.tsx:10
- [nitpick] The naming of the imported store is inconsistent with other parts of the code. Consider renaming this import to 'useTaskSelectionStore'.
import useTodoSelectionStore from '@/store/useTaskSelectionStore';
seong-hui
left a comment
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.
task가 여러 곳에서 연관되어 사용되어지는만큼 클릭된 task관리를 위해서 전역상태 도입은 굉장히 좋은 선택인거 같아요! 확실히 선택한 task를 구별할 수 있으니 사용하면서 직관적이고 좋으네요! 드랍으로 캘린더에 일정을 생성하는 부분은 더 고민을 해보면 좋을 것 같습니다! 해당 기능이 없어도 서비스를 이용하기엔 무리가 없지만 약간 아쉽기도..? ㅋㅋㅋㅎ 이번에 어려우면 추후에 개발해도 좋을 것 같아요~~ 고생 많으셨습니다 👍
| "react-router-dom": "^6.24.1", | ||
| "vite-plugin-svgr": "^4.3.0" | ||
| "vite-plugin-svgr": "^4.3.0", | ||
| "zustand": "^5.0.3" |
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.
전역상태관리를 라이브러리로 zustand를 선택하신 이유가 궁금해요! (정말 단순 궁금)
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.
jotai랑 고민했는데, 러닝커브나 탑다운방식이 조금 더 익숙해서 선택했습니다! 선택된 테스크를 전체페이지에서 공유하는만큼 좀 더 어울린다고 생각하기도 했구요
src/hooks/useOutsideTaskClick.ts
Outdated
| const { clearSelectedTask, selectedTask } = useTaskSelectionStore(); | ||
|
|
||
| useEffect(() => { | ||
| const handleMouseDown = (event: MouseEvent) => { |
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.
드롭을 통한 이벤트생성을 위해서 외부영역 클릭 시 clearSelectedTask()을 호출하기 전에 calendarRef.current 예외처리를 한것처럼 isDragging의 상태가 true이면 return되는 식으로 구현하는 건 어떨까요?
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.
오오 수정한 부분 확인했습니다. 다만 해당 수정과정에서 캘린더 영역의 task 더블 클릭이 막힌것 같아요 확인 부탁드립니다!!
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.
헉 꼼꼼한 체크 감사합니다!!!👍 👍 👍 수정하였습니다
| setIsDragging: (dragging: boolean) => void; | ||
| } | ||
|
|
||
| const useTaskSelectionStore = create<TaskSelectionState>((set) => ({ |
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.
zustand는 사용해보지 않았는데 이런식으로 이렇게 상태를 관리하는군요 직관적이고 좋으네용 👍
5f5e4c7 to
4b33c9d
Compare
작업 내용 🧑💻
useTaskSelectionStore)selectedTarget->selectedTask로 상태명 명확하게 변경setSelectedTask,clearSelectedTask를 통해 선택된 task 관리((Today 컴포넌트의useOutsideTaskClick)handleClickOutside)clearSelectedTask)알게된 점 🚀
리뷰 요구사항 💬
clearSelectedTask()을 호출하려면(: 클릭 잃게 하려면) 풀캘린더의 외부아이템 드롭을 통한 이벤트생성을 못하고있는데, 좋은 방법 떠오르시면 답글 부탁드립니다!관련 이슈
close #435
스크린샷 (선택)