-
Notifications
You must be signed in to change notification settings - Fork 2
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
[시간표] 커스텀 시간표 페이지 리팩터링 #656
Open
Gwak-Seungju
wants to merge
25
commits into
develop
Choose a base branch
from
fix/#650/refactor-custom-lecture
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChoiWonBeen
reviewed
Feb 13, 2025
ChoiWonBeen
reviewed
Feb 13, 2025
Comment on lines
84
to
90
const findKeyByValue = (object: Record<Hour, number>, value: number) => Object | ||
.entries(object).find(([, val]) => val === value)?.[0] as Hour; | ||
const getHour = (time: number, key: Record<Hour, number>, isStart: boolean) => { | ||
const adjustedTime = time % 2 === (isStart ? 0 : 1) ? time : time - 1; | ||
return findKeyByValue(key, adjustedTime); | ||
}; | ||
const getMinute = (time: number, isStart: boolean): Minute => (time % 2 === (isStart ? 0 : 1) ? '00분' : '30분'); |
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.
이런 함수는 react의 생명주기 안에 있을 필요가 없는 것 같아욤
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.
앗! 그렇네요! 반영했습니다! 2e8fccf
…/BCSDLab/KOIN_WEB_RECODE into fix/#650/refactor-custom-lecture
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this PR? 🔍
Changes 📝
@ChoiWonBeen 님의 피드백을 받고 정리한 관련 노션입니다.
관련 스레드입니다.
아무래도 노션에서 정리한 5번과 6번을 중점적으로 리뷰해주시면 될 것 같습니다.
5번 내용은 지금까지
CustomLecture
라는 폼 컨테이너에 모든 state를 넣고 관리했는데 시간과 장소를 묶는 컨테이너를 컴포넌트화할 필요가 있다하여TimeSpaceInput
이라는 이름으로 컴포넌트화했습니다.6번 내용은 useEffect를 줄일 수 있으면 줄여라 였는데, 결과적으로는 더 늘리는 꼴이 되었습니다. 그 이유는 아래와 같습니다.-> 해결했습니다.customTempLecture라는 현재 커스텀 일정의 정보를 담고 있는 객체를 전역 상태로 관리하고 있습니다. 이 객체가 하는 역할은 그 강의 정보를 시간표에 미리보기볼 수 있도록 하는 것입니다. 따라서 form을 업데이트 할 때마다 customTempLecture도 즉각 업데이트를 해줘야 하는데, 컴포넌트를 분리하다 보니 자식, 부모, 컴포넌트에서 updateCustomLecture를 해야 하는 상황이 있었습니다.
"한 줄 요약"
컴포넌트화해서
솔직히 잘 리팩터링한 건 지는 모르겠습니다... 코드 길이도 이전보다 100줄 정도 늘어나기도 해서..
✔️ Please check if the PR fulfills these requirements
develop
branch unconditionally?main
?yarn lint