Skip to content
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

Merged
merged 11 commits into from
Sep 4, 2024

Conversation

wuzoo
Copy link
Contributor

@wuzoo wuzoo commented Sep 3, 2024

Summary

close #406

Screenshot

2024-09-03.6.51.43.mov

Comment

  • 상단 배너 업데이트
  • 네비 바 "지원하기" 클릭 시 리크루팅 페이지로 이동
  • 키컬러 반영
  • 모집일정 반영
  • 하단 배너 업데이트
  • 기존 useCheckTime 수정

@wuzoo wuzoo self-assigned this Sep 3, 2024
Copy link

height bot commented Sep 3, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link
Member

@eonseok-jeon eonseok-jeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파트별 인재상과 QnA 부분은 아직 반영 안 된 거 맞죠??

고생하셨습니다 :)

아 깜빡하고 말씀 못드린 거 같은데 커밋 컨벤션은 기능: 설명 으로만 하고 있어요!
e.g. feat: 로그인 기능 구현

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

34기 YB 지원하기 -> 35기 YB 모집하기로 바꾸면 좋을 거 같아요 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요친구는 메인페이지 작업이니까 메인페이지 작업할 때 반영해줘도 될 것 같아요 ! !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 메인페이지 작업할 때 처리할 것이였어서 그 때 반영하겠습니다 !

Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
);
};
const validate = setInterval(check, 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모집 기간이 마감되었을 때 한 번만 체크해주면 되니까 setInterval 보단 setTimeout이 더 나을 거 같아요 :)
setTimeout(check, 마감 시간 - 현재 시간) 이런 식으로 말이죠
매초 마다 check 실행시켜주면 메모리도 낭비되니까요!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그으으으런데 제가 보기엔
여기서 주용오빠가 setInterval을 사용한 이유는 모집알림신청 -> YB 지원하기 변경되는게 리로드 기준이 아니라 정각되면 땡 ! 하고 바뀌길 바래서 interval을 쓰지 않았을까 싶어요. 그런데 이걸 setTimeout으로 바꾸면 땡 ! 이 안되니까 주용오빠 의도를 충족시킬 수 없지 않을까요??

성능을 고려한다면 제 갠적 의견으로는 아예 타이머를 제외시켜도 될 것 같아요.
이게 막 지원서 제출 시각 처럼 1초 1초가 엄청 크리티컬한 이벤트도 아니라서 사용자 리로드에 의존하는게 어떨까 . . 하는 생각도 듭니다용

다른분들 의견은 어떠신지 ~~~

Copy link
Member

@eonseok-jeon eonseok-jeon Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setTimeout 써도 땡 돼요! 테스트 해봤습니당~

이게 1초마다 매번 실행이 되어야 자동으로 바뀌는 것이 아니고
그렇게 계속 1초마다 실행되다가 마감 시간 됐을 때도 실행이 되었기에 UI가 변경이 되는 거라
마감 시간 되었을 때만 딱 한 번 실행이 된다면 동일한 효과를 얻을 수 있어요!

그리고 setTimeout 써도 delay 후에 한 번만 실행되는 거라 성능 상에 큰 문제는 없을 거 같아요 ~~

하지만 setInterval, setTimeout 다 제외하는 것도 좋습니다~ UI 바로 안 변한다 하더라도 마감 시간 지나고 나서 링크 눌렀을 때 지원하기 사이트에서 이미 접근이 막힐테니까요

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 제가 기존의 useEffect 내에서의 setter 함수를 두 번 호출하는 코드를 지금과 같이 수정한 이유는,
Time 이라는 외부 시스템에 대한 상태를 관리하기 위해 상태를 두 번 선언하는 것보다는 차라리 원래 Effect의 취지에 맞게 브라우저 API(외부 API)를 Effect를 통해 동기화한다라는 것이 더욱 React 적으로 올바르다 라고 생각해서이긴 했습니다.

하지만, 두 분 말씀대로 그리 크리티컬한 이벤트도 아니고, useEffect 안에서 커밋이 이루어지자마자 다시 상태 변경에 의해 리렌더링되는 안티 패턴을 기술적으로 고려한다고 해도 문제가 많지 않기 때문에, 그냥 기존의 코드대로 가도 될 거 같긴 하네요..

  • 추가적으로, 근데 언석님께서 말씀하신 마감 시간 - 현재 시간setTimeout의 딜레이로 설정한다면, 땡 ! 이 안되지 않나요 ? 주기적으로 check 함수를 타임아웃을 통해 구현하려면 setTimeout을 중첩해서 선언해야 하는 것으로 알고 있습니다..!

만약 현재 사용자가 "리크루팅 페이지"에 들어온 시점이 start 1분전이고 마감시간이 start로부터 1주일 후라면, 약 1주일 후에 setTimeout의 콜백이 한 번 실행될 걸로 예상되는데 언석님께서 테스트 해보셨다고 하니 어떻게 하셨는지 궁금하네요 !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 1분 1초의 상황을 다루는 이벤트가 아니므로, 기존의 방식을 따른다면 다음과 같이 반영할 예정입니다. 확인해주세용 !

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eonseok-jeon

제가 인자를 넣어준 곳은 useTimeInRange가 아닌 setTImeout 입니다! useTimeInRange의 인자는 바꾸지 않았어요

혹시 UI바로 바로 바뀌는지 영상 찍어주신 것에서

useTimeInRange("2024-09-02 10:00:00", "2024-09-04 16:50:50")

로 사용하신 것처럼 보이는데, 결국에는 endDate에 현재 시간이 다다르냐! 로 판단을 하신거라고 이해를 했습니다 !

하지만 저는 endDate에는 모집 일정의 마감 기간인 실제 2024-09-13 18:00:00를 넣어주었고, startDate에는 모집 시작 기간(실제 2024-09-08 10:00:00, 하지만 시작 기간을 지났을 때 UI가 바뀌는 것을 판단하기 위해 언석님과 같이 2024-09-04 16:50:50테스트 시간)을 넣어주었던 것 같습니다.

결국 시작 시간, 마감 시간을 해당 훅에 넣어주셨고 마감 시간 - 현재 시간setTimeout 함수의 delay 에 넣어주셨지만,
제 의도는 시작 시간을 이미 지났고 마감 시간에 다다르냐를 테스트하는 것이 아니라, 시작 시간에 다다르었을 때 UI가 바뀌냐를 테스트한 것 같아요 !
따라서 setTimeout이 아닌 setInterval을 택하였습니다 !

Copy link
Member

@lydiacho lydiacho Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👩🏻‍⚖️

  • 언석님 의견 : UI가 렌더링된 시점부터 마감시간 까지의 차이를 계산해서, 그 차이만큼 delay가 지난 후 땡! 하고 UI가 바뀌도록 구현.
  • 주용님 의견 : 언석님의 방식은 마감시간에 땡! 바뀌는 기능만 해주는데, 시작시간에 땡! 바뀌는 것은 기능하지 않다. check의 역할은 시작시간~마감시간 사이에 속하냐를 체크하는 역할이기 때문에, 언석님이 구현해주신 마감시간이 되엇는지여부만 가능해서는 본 기능을 다하지 못한다

Copy link
Member

@lydiacho lydiacho Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

두분 의견 다 완전 이해했습니다
그럼 언석님 + 주용님 절충안을 뽑아보자면

setTimeout(check, 시작시간 - 현재시간); 
setTimeout(check, 마감시간 - 현재시간); 

그냥 이렇게 타임아웃 두번 돌려주면 되는 일인 것 같은데
아까 결론 난 것처럼 굳이 타이머로 체크해줄 필요 없는 친구라고 의견이 모아졋으니 주용님 마지막에 반영한 코드로 가면 될듯요 ~~

Copy link
Member

@eonseok-jeon eonseok-jeon Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wuzoo 아항 이해했습니다~

그것도 사실 비슷한데요
마감 시간에 땡 할 수 있으면 시작 시간 전에도 땡 해줄 수 있어요

timeout 하나 더 추가하는 게 매 초 시작해주는 것보다 메모리 성능적으로 더 효율적일 거 같아용~~

const isValid = useTimeInRange('2024-09-04 17:49:00', '2024-09-04 17:49:10');
테스트는 17:49분 전에 시작했어요

2024-09-04.5.47.08.mov
코드 보기
import { useEffect, useState } from 'react';

export default function useTimeInRange(startDate: string, endDate: string) {
  const [isValid, setIsValid] = useState(
    Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
  );

  useEffect(() => {
    const check = () => {
      setIsValid(
        Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
      );
    };
    const validate2 = setTimeout(check, new Date(startDate).getTime() - Date.now());
    const validate = setTimeout(check, new Date(endDate).getTime() - Date.now());

    return () => {
      clearTimeout(validate);
      clearTimeout(validate2);
    };
  }, [startDate, endDate]);

  return isValid;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 넵 ! 이해했습니다 좋은 피드백 감사드립니당

최종으로는 아까 말씀드린 기존 방식으로 반영하겠습니답

Copy link
Member

@lydiacho lydiacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다 ~~ 👍

파트별 인재상 해당 브랜치에서 잊지말고 착업해주세요!
그리고 FAQ는 임원진분들이 따로 업데이트 할 내용 전달 안해주셨는데,
지난 기수에는 수정사항이 있었어서 혹시 이번에도 필요할지 다시한번 체크하면 좋을 것 같아서 임원진 측에 슬랙으로 질문해놓았어요

답 기다렸다가 수정 필요하다고 하면 그거까지 같이 반영해줍시당 ~~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요친구는 메인페이지 작업이니까 메인페이지 작업할 때 반영해줘도 될 것 같아요 ! !

Comment on lines 4 to 6
const [isValid, setIsValid] = useState(
Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startDate, endDate는 props라서 어차피 값이 바뀔 시 useTimeInRage가 다시 실행되니까
데이터 파싱을 useEffect 내부 해주지 않고 한번만 만들어줘도 되지 않을까유??

Suggested change
const [isValid, setIsValid] = useState(
Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
);
const start = new Date(startDate).getTime();
const end = new Date(endDate).getTime();
const [isValid, setIsValid] = useState(
Date.now() >= start && Date.now() <= end,
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞네요 ! 계산 덜 할 수 있을 것 같아요. 반영하겠습니당

Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
);
};
const validate = setInterval(check, 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그으으으런데 제가 보기엔
여기서 주용오빠가 setInterval을 사용한 이유는 모집알림신청 -> YB 지원하기 변경되는게 리로드 기준이 아니라 정각되면 땡 ! 하고 바뀌길 바래서 interval을 쓰지 않았을까 싶어요. 그런데 이걸 setTimeout으로 바꾸면 땡 ! 이 안되니까 주용오빠 의도를 충족시킬 수 없지 않을까요??

성능을 고려한다면 제 갠적 의견으로는 아예 타이머를 제외시켜도 될 것 같아요.
이게 막 지원서 제출 시각 처럼 1초 1초가 엄청 크리티컬한 이벤트도 아니라서 사용자 리로드에 의존하는게 어떨까 . . 하는 생각도 듭니다용

다른분들 의견은 어떠신지 ~~~

Copy link
Contributor Author

@wuzoo wuzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가적으로 아까 말씀드린 파트별 인재상과 FAQ 반영하도록 하겠습니답

Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime(),
);
};
const validate = setInterval(check, 1000);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이해했습니다 언석님 !!

처음 언석님이 언급하셨을 때 마감시간 - 현재시간이라고 하셔서 이해가 안됐는데, 지금 구현 코드 보여주신거는 시작시간 - 현재시간처럼 보여요 !

그렇다면 언석님 말씀대로 구현 가능할 것 같네요 !

Copy link
Member

@eonseok-jeon eonseok-jeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨어요~~~ :)

Comment on lines 9 to 11
useEffect(() => {
setIsValid(Date.now() >= start && Date.now() <= end);
}, [start, end]);
Copy link
Member

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 해줄 필요는 없어 보여요~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇긴 하네요 ! 사실 prop이 변경되었을 때 또한 트리거하기 위해서 useEffect를 사용한건데, 언석님 말씀대로 지원 시작 기간과 종료기간을 상태가 아닌 그냥 상수로 넣어줄거라면 필요가 없을 것 같긴 합니다 !

만약 useEffect를 제외할거라면 해당 커스텀 훅을 그냥 삭제시키고 사용하는 쪽에서 다 적어주어도 무방할거 같다고 생각하는데, 다들 어떻게 생각하시나용

Copy link
Member

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으로 분리하여 관리하는 것이 더 깔끔할 거 같단 생각입니다!

하지만 아직까진 두 군데서 밖에 쓰이지 않고 추후에도 어딘가 이를 이용할 곳이 더 생기진 않을 거 같아 주용님 편하신 대로 해도 좋습니다~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇다면 숭희만 어푸하면 이대로 머지하겠슴니당

Copy link
Member

@eonseok-jeon eonseok-jeon Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 useEffect는 빼고 말한 거였어요!
useEffect 제외한 나머지 부분을 hook으로 그대로 유지한 채 사용이었습니다!!
제가 아까부터 계속 표현을 모호하게 한 거 같네요ㅜ 좀 더 확실하게 표현하도록 노력해볼게요!! 😅

Copy link
Member

@eonseok-jeon eonseok-jeon Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

첫 렌더링 시 setter 함수에서 초기화 된 값과 동일한 값을 useEffect에서 다시 set 해주는 건데 어떤 면에서 사용자 측면에 유연한 건지 조금 잘 모르겠어요ㅜ useEffect를 넣음으로써 어떻게 유연성이 확장이 되는 지 좀 더 설명해 주실 수 있으신가요???

Copy link
Member

@lydiacho lydiacho Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 그런데용
현재 저희가 결정한대로면 사용자의 리로드에 의존해서 해당 훅이 실행됐을 때 isValid한 시간인지 체크하기로 했잖아용
그럼 useEffect 뿐만 아니라 useState도 그냥 싸그리 없애버려도 되는거 아닌가요?


현재 저희 코드가

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;
}

이렇게 생겼는데

  • 언석님이 말씀하시는대로 현재 useTimeInRange의 props가 startDate, endDate 이기 때문에 해당 값들이 바뀌면 알아서 useTimeInRange는 재실행될거고 (어차피 상수라 그럴일없겠지만)
  • start, end 값은 startDate, endDate에서 나온 애들이니까 useEffect에서 의존성배열로 체킹해줄 필요 없고
  • 타이머 체크 안하고 리로드하기로 했으니까 isValid 여부를 체크해서 동적으로 렌더링시켜줄 필요도 없어진거 같아요

즉 얘가 해줄 일은 단순히

시작일시, 마감일시를 입력했을 때, 실행 시 valid한 시간인지 뱉어주기

이거 밖에 안하게될 것 같아요


그래서 싸그리 빼버리고

const checkTimeInRange = (startDate: string, endDate: string) =>
  Date.now() >= new Date(startDate).getTime() && Date.now() <= new Date(endDate).getTime();

이렇게 valid한 날짜인지 boolean으로 뱉어주는 간단한 유틸함수로 바꿔주는게 낫지 않을까유?

Copy link
Member

@eonseok-jeon eonseok-jeon Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lydiacho 오홍? 그것도 그렇네요!? 제 시야가 너무 좁게 잡혔던 거 같아여 😓 좋은 거 같습니다~~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

두분 말씀 다 이해했습니다. 저 또한 생각이 좀 짧았던 것 같네요.

  1. 저는 일단 인자가 상태 값으로 변경되었을 시에도 값을 최신화하고 싶었던 생각이였는데, 어차피 해당 커스텀 훅 소비자의 상태가 바뀐다면 해당 커스텀 훅도 재실행될테니 useEffect도 필요가 없어질 것 같아요. 그렇다면 Date에 의한 isValid도 최신화가 되겠네요
  2. 따라서 승희 말대로 어차피 리로드에 의존할 것이므로 상태로 작성해줄 필요도 없겠네요.

이해 완료했습니다 ~ 피드백 감사드려요.

그럼 최종 방향대로 유틸 함수로 작성해서 다시 반영할까요 ?

Copy link
Member

@lydiacho lydiacho Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네네 좋습니두 유틸함수 반영된거 보이면 바로 어푸 날릴게여 ! ! !
고생하셨습니당 ~~

Copy link
Member

@lydiacho lydiacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파일명을 date로 하고 default export 안쓴건 추후 날짜 관련된 유틸함수를 해당 파일에 모아줄 계획인것이겟쥬?

확인했습니다 고생하셨습니두 ~~~ 🚀 🚀

@wuzoo wuzoo merged commit c86613f into develop Sep 4, 2024
1 check passed
@wuzoo wuzoo deleted the feat/#406_35-recruit branch September 4, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 리크루팅 페이지 업데이트
3 participants