refactor(time): 시간표 페이지 디자인 수정 #281
Conversation
…이지 컴포넌트에서 세부 너비 조절
|
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #281 +/- ##
==========================================
- Coverage 94.06% 93.71% -0.35%
==========================================
Files 14 14
Lines 219 207 -12
Branches 37 38 +1
==========================================
- Hits 206 194 -12
Misses 11 11
Partials 2 2 see 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Jeong-Ag
left a comment
Jeong-Ag
left a comment
There was a problem hiding this comment.
디자인 수정 PR이나 코드 리팩토링 관련은 별도 PR로 빼는게 좋을 것 같네요 간략하게만 남겨서 추후 리팩토링 진행할때 더 자세히 남길게요 스타일링 관련한 리뷰만 반영해주셔도 돼요 수고하셨습니다~ 🙌
| interface TimeTableProviderProps { | ||
| children: ReactNode; | ||
| } |
There was a problem hiding this comment.
TimeTableContext 위로 올려서 interface 모아놓으면 더 좋을 것 같아용
| const clearSelectedTimeInfo = () => { | ||
| setSelectedTimeInfo(null); | ||
| }; |
There was a problem hiding this comment.
setSelectedTimeInfo 전달하니까 꼭 필요한 함수는 아니라고 생각하는데 명확하게 전달하기 위해서 넣으신걸까요?
| const hasTimeConflict = (time1: string, time2: string): boolean => { | ||
| const periods1 = parseLectureTime(time1); | ||
| const periods2 = parseLectureTime(time2); | ||
|
|
||
| return periods1.some((period1) => | ||
| periods2.some((period2) => period1 === period2), | ||
| ); | ||
| }; |
| const router = useRouter(); | ||
| const { searchParamsAction } = useTimeTableParams(); | ||
|
|
||
| const addedLectureIds = useMemo(() => { |
There was a problem hiding this comment.
단순 파싱같은데 메모이제이션 한 이유가 궁금해요 다른 함수들에도 적용되어 있는 메모이제이션이 꼭 필요하지 않을 수도 있다는 생각이 들어서 확인해보면 어떨따요
|
|
||
| const addLecture = useCallback( | ||
| (lecture: GetLectureListResponseValue) => { | ||
| const currentParams = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
useSearchParams을 사용하지 않고 window.location.search를 사용한 이유가 있을까요? url이 변경됐을때 감지가 안될 것 같아요
| }, | ||
| set: (key: string, value: string) => { | ||
| searchParams.set(key, value); | ||
| router.push(`${PATH.TIMETABLE}?${searchParams.toString()}`); |
| }, | ||
| ); | ||
|
|
||
| LectureSearchPeriodDropdown.displayName = 'LectureSearchPeriodDropdown'; |
There was a problem hiding this comment.
코드 아래에 모아서 작성하면 관리하기 더 편할 것 같아요
| className={`px-3 py-1 text-sm transition-colors hover:bg-gray-300/20 ${ | ||
| selectedSet.has(item) | ||
| ? 'bg-time-table-header' | ||
| : 'bg-white text-gray-700' | ||
| }`} |
There was a problem hiding this comment.
| className={`px-3 py-1 text-sm transition-colors hover:bg-gray-300/20 ${ | |
| selectedSet.has(item) | |
| ? 'bg-time-table-header' | |
| : 'bg-white text-gray-700' | |
| }`} | |
| className={cn('px-3 py-1 text-sm transition-colors hover:bg-gray-300/20', selectedSet.has(item) ? 'bg-time-table-header' : 'bg-white text-gray-700')} |
코드 전체적으로 cn 함수 사용해서 조건부 스타일 적용해주시면 될 것 같아요
| if (isAddedLecture) { | ||
| deleteLecture(lecture.id); | ||
| return; | ||
| } | ||
|
|
||
| if (isTimeConflict) { | ||
| alert('이미 추가된 강의와 시간이 충돌합니다.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if (isAddedLecture) { | |
| deleteLecture(lecture.id); | |
| return; | |
| } | |
| if (isTimeConflict) { | |
| alert('이미 추가된 강의와 시간이 충돌합니다.'); | |
| return; | |
| } | |
| if (isAddedLecture) { | |
| return deleteLecture(lecture.id); | |
| } | |
| if (isTimeConflict) { | |
| return alert('이미 추가된 강의와 시간이 충돌합니다.'); | |
| } |
| <LectureSearchItem.Dropdown | ||
| title="시간 선택" | ||
| isOpen={isOpen} | ||
| onToggle={() => setIsOpen(!isOpen)} |
There was a problem hiding this comment.
| onToggle={() => setIsOpen(!isOpen)} | |
| onToggle={() => setIsOpen(prev => !prev)} |
Summary
기존 작성되어 있던 시간표 관련 로직에 대해 재컴포넌트화, 커스텀 hook 생성 등을 통해 리팩토링했습니다.
Tasks
To Reviewer
Screenshot