-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Feat] 35기 YB 리크루팅 페이지 업데이트 #407
Changes from 10 commits
02b5f77
43272c4
8731198
f872a4d
9774823
ae9c94d
7e4935e
09b1574
da72062
968cee4
f074ea0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { useEffect, useState } from 'react'; | ||
|
||
export default function useTimeInRange(startDate: string, endDate: string) { | ||
const start = new Date(startDate).getTime(); | ||
const end = new Date(endDate).getTime(); | ||
|
||
const [isValid, setIsValid] = useState(Date.now() >= start && Date.now() <= end); | ||
|
||
useEffect(() => { | ||
setIsValid(Date.now() >= start && Date.now() <= end); | ||
}, [start, end]); | ||
|
||
return isValid; | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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.
뒷북이긴 한데 사실 useEffect 사용 안 해도 될 거 같아요!
처음 useTimeInRange가 실행되면 isValid가 Date.now() >= start && Date.now() <= end 로 초기화 되기 때문에 useEffect를 이용해서 한 번 더 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.
그렇긴 하네요 ! 사실
prop
이 변경되었을 때 또한 트리거하기 위해서useEffect
를 사용한건데, 언석님 말씀대로 지원 시작 기간과 종료기간을 상태가 아닌 그냥 상수로 넣어줄거라면 필요가 없을 것 같긴 합니다 !만약
useEffect
를 제외할거라면 해당 커스텀 훅을 그냥 삭제시키고 사용하는 쪽에서 다 적어주어도 무방할거 같다고 생각하는데, 다들 어떻게 생각하시나용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.
그렇게 해도 무방하겠지만 저의 의견을 짧게 말씀드리자면,,,
해당 동일 로직이 Main page와 Recruit Page 둘 다에서 사용되고 있기에,
각 페이지 컴포넌트에서 로직을 분리하여 custom hook으로 관리하고 또 해당 hook을 재사용할 수 있단 측면에서
큰 기능이 없다 하더라도 custom hook으로 분리하여 관리하는 것이 더 깔끔할 거 같단 생각입니다!
하지만 아직까진 두 군데서 밖에 쓰이지 않고 추후에도 어딘가 이를 이용할 곳이 더 생기진 않을 거 같아 주용님 편하신 대로 해도 좋습니다~~~~
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.
아 useEffect는 빼고 말한 거였어요!
useEffect 제외한 나머지 부분을 hook으로 그대로 유지한 채 사용
이었습니다!!제가 아까부터 계속 표현을 모호하게 한 거 같네요ㅜ 좀 더 확실하게 표현하도록 노력해볼게요!! 😅
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.
첫 렌더링 시 setter 함수에서 초기화 된 값과 동일한 값을 useEffect에서 다시 set 해주는 건데 어떤 면에서 사용자 측면에 유연한 건지 조금 잘 모르겠어요ㅜ useEffect를 넣음으로써 어떻게 유연성이 확장이 되는 지 좀 더 설명해 주실 수 있으신가요???
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.
음 그런데용
현재 저희가 결정한대로면 사용자의 리로드에 의존해서 해당 훅이 실행됐을 때 isValid한 시간인지 체크하기로 했잖아용
그럼 useEffect 뿐만 아니라 useState도 그냥 싸그리 없애버려도 되는거 아닌가요?
현재 저희 코드가
이렇게 생겼는데
startDate
,endDate
이기 때문에 해당 값들이 바뀌면 알아서 useTimeInRange는 재실행될거고 (어차피 상수라 그럴일없겠지만)즉 얘가 해줄 일은 단순히
이거 밖에 안하게될 것 같아요
그래서 싸그리 빼버리고
이렇게 valid한 날짜인지 boolean으로 뱉어주는 간단한 유틸함수로 바꿔주는게 낫지 않을까유?
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.
@lydiacho 오홍? 그것도 그렇네요!? 제 시야가 너무 좁게 잡혔던 거 같아여 😓 좋은 거 같습니다~~
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.
두분 말씀 다 이해했습니다. 저 또한 생각이 좀 짧았던 것 같네요.
useEffect
도 필요가 없어질 것 같아요. 그렇다면 Date에 의한isValid
도 최신화가 되겠네요이해 완료했습니다 ~ 피드백 감사드려요.
그럼 최종 방향대로 유틸 함수로 작성해서 다시 반영할까요 ?
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.
네네 좋습니두 유틸함수 반영된거 보이면 바로 어푸 날릴게여 ! ! !
고생하셨습니당 ~~