[시간표] 빈 강의 목록에 대한 문구 추가 #519
Conversation
There was a problem hiding this comment.
겸사겸사 코드를 읽어봤는데 시간표 코드가 전체적으로 읽기 어려운 부분이 있네요..
우선 순위에서 밀렸을 수 있지만 이벤트 effect와 아닌 effect, hooks와 데이터 가공을 위한 로직, jsx를 쉽게 파악할 수 있는 구조를 그린다음 이에 맞는 이름을 잘 붙혀주기만 해도 가독성이 크게 오를거에요!
급한 내용이라고 했는데 느긋하게 리뷰를 남겨서 조금 죄송하지만, 분명 도움이 되리라고 생각합니다!
우선 기능적 오류는 보이지 않아서 approve 해두겠습니다! 뒤늦게라도 참고해 봐도 괜찮아요!
정말 열심히 하시던데... 응원해요!😆
| value: LectureInfo | TimetableLectureInfoV2, | ||
| ): value is LectureInfo => 'name' in value; | ||
|
|
||
| export const LECTURE_TABLE_HEADER = [ |
There was a problem hiding this comment.
이 constant는 다른곳에서도 쓰이는 것 같네요?
저라면 선언 위치가 올바른지 고민해볼것 같아요.
다시 보니 코인이 constant를 관리하는 구조가 제대로 안 되어 있는 느낌도 있군요..
There was a problem hiding this comment.
생각해보니 static 폴더에 timetable.ts가 있네요! 거기에 선언을 하면 더 좋을까요??
| [styles['table__row-button--toggled']]: version === 'myLectureList', | ||
| })} | ||
| onClick={(e) => { | ||
| setCursor(index); |
There was a problem hiding this comment.
index말고 list가 가지는 고유 값으로 Cursor를 구분할 수는 없을까요?
index를 사용하는 용도가 특정 엘리먼트를 찾기 위함이라면 지양하는 습관을 들이는게 좋은 것 같아요!
There was a problem hiding this comment.
cursor가 사용되는 경우가 키보드 방향키 위, 아래를 눌렀을 때 LectureList 중 클릭해서 선택한 lecture를 기준으로 위, 아래로 이동이 될 때 사용됩니다! 따라서 index를 사용했습니다..!
| list={(lectureList ?? []).filter((lecture) => { | ||
| const searchFilter = filter.search.toUpperCase(); | ||
| const departmentFilter = filter.department; | ||
| const searchCondition = lecture.name.toUpperCase().includes(searchFilter) | ||
| || lecture.code.toUpperCase().includes(searchFilter) | ||
| || lecture.professor.toUpperCase().includes(searchFilter); |
There was a problem hiding this comment.
해당 작업은 연산이 많아보이네요..
list의 크기가 충분히 커질 수 있는 작업이라면 메모이제이션을 생각해봐도 좋을 것 같고, jsx 내부에 있어서 읽기가 힘드니 따로 꺼내서 선언해 네이밍을 해줌으로써 결과값을 수월하게 읽을 수 있도록 하면 좋을 듯 합니다~!
| onClick={(e) => { | ||
| setCursor(index); | ||
| handleTableRowClick(lecture, e); | ||
| }} |
There was a problem hiding this comment.
좀 개선해 보고 싶은 구조네요.
커링과 list가 가지는 고유값을 사용한다면 이벤트의 특성을 살리면서 핸들러 함수도 깔끔하게 분리가 될 것 같단말이죠..!
아마
const handleTableRowClick = (e) => (lecture) => {...}
...
onClick={handleTableRowClick(lecture)}
이런 느낌으로 떨어질 것 같습니다!
What is this PR? 🔍
Changes 📝
코드 이동으로 추가 코드가 많은 만큼 삭제 코드도 많습니다. 변경한 내용은 별로 없습니다!
ScreenShot 📷
-빈 강의 목록
✔️ Please check if the PR fulfills these requirements
developbranch unconditionally?main?yarn lint