-
Notifications
You must be signed in to change notification settings - Fork 8
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
[FE] 매칭 완료, 피드백 완료 알림 기능(#773) #780
Conversation
- refetchInterval: 1분마다 재요청 - refetchIntervalInBackground: 탭이 활성화되어 있을 때만 재요청
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.
안녕하세요 초코🍫알림 종류가 많아지면서 리팩토링에 시간을 많이 쏟은 게 느껴졌어요!! 수고 많으셨습니다👏👏
서버 오류로 피드백 페이지로 리다이렉트 되는 부분을 확인하지 못 했어요.. 오류 해결하는 대로 확인해보겠습니다!
오류 해결되기 전에 프론트가 작업하면 좋을 부분 몇 개 있어서 우선 RC 드렸습니다..! 코멘트 확인해주세요😊
frontend/src/@types/alaram.ts
Outdated
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.
파일 이름 오타인 것 같아요! alarm.ts
Code Spell Checker 익스텐션 설치하면 오타 잡기 쉽습니당ㅎㅎ
const getAlarmMessage = () => { | ||
if (actionType === "MATCH_COMPLETE" || actionType === "MATCH_FAIL") { | ||
const message = ALARM_MESSAGES_WITHOUT_USERNAME[actionType]; | ||
return message?.(interaction.info) ?? <></>; | ||
} | ||
|
||
const message = ALARM_MESSAGES_WITH_USERNAME[actionType]; | ||
return message?.(actor?.username ?? "", interaction.info) ?? <></>; | ||
}; |
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.
const getAlarmMessage = () => { | |
if (actionType === "MATCH_COMPLETE" || actionType === "MATCH_FAIL") { | |
const message = ALARM_MESSAGES_WITHOUT_USERNAME[actionType]; | |
return message?.(interaction.info) ?? <></>; | |
} | |
const message = ALARM_MESSAGES_WITH_USERNAME[actionType]; | |
return message?.(actor?.username ?? "", interaction.info) ?? <></>; | |
}; | |
const getAlarmMessage = () => { | |
const message = ALARM_MESSAGES[actionType]; | |
if (!message) return <></>; | |
return message({ | |
username: actor?.username, | |
info: interaction.info, | |
}); | |
}; |
type AlarmMessageProps = {
username?: string;
info?: string;
};
이렇게 props를 객체로 받으면 두 개의 객체를 하나의 객체로 줄일 수 있을 것 같아요!!!
또 ALARM_MESSAGES는 상수 파일로 분리해도 괜찮을 것 같네요😊
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.
ALARM_MESSAGES로 객체를 합치고, 상수 파일로 분리했습니다~
(기존에는 constants에 ts파일만 있어서 갑자기 tsx 파일이 추가되는 게 어색해서 안했음)
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.
span 때문에 tsx 파일이 되는군요...! 더 좋은 방법으로 분리할 수 없을까 생각했는데 딱히 떠오르지 않네요ㅠㅜㅠㅜ 변경 내용은 확인 했습니다!!👍👍
export const useFetchAlarmCount = (enabled: boolean = false) => { | ||
return useQuery({ | ||
queryKey: [QUERY_KEYS.ALARM_COUNT], | ||
queryFn: getAlarmCount, | ||
enabled, | ||
refetchInterval: 60000, | ||
refetchIntervalInBackground: false, |
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.
언마운트 됐을 때는 해당 api를 호출하지 않게 하려고 refetchIntervalInBackground: false
를 추가한 건가요???
그리고 가독성을 위해 60*1000으로 하는 거 어떤가용ㅎㅎㅎ??
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.
언마운트보다는 우리 서비스의 탭이 포커싱이 되지 않았을 때 refetch 하지 않도록 하는 설정인거 같아요!
refetchIntervalInBackground
의 기본값이 false 이기에 적어주지 않아도 될것 같지만 자주 사용하지 않는 옵션이라 명시적으로 적어줘서 생각해볼 수 있게 해줘서 좋은거 같아요 ㅎㅎ
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.
실제로 다른 탭을 포커싱 해두고 작은 화면으로 확인했을 땐 계속 refetch를 하고 있더라구요..! 그리고 메인 페이지에 있을 때는 alarm 리스트를 refetch 하지 않았습니다!
우리 서비스의 탭이 포커싱이 되지 않았을 때
저도 이렇게 알고 있었는데 제가 알고 있는 게 틀린 건가? 싶어서 질문 드렸어요. 제가 실험을 잘못한 걸까요..???
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.
맞아요! 커밋내역에도 적어놓았고, 다르가 얘기한 것처럼 탭이 활성화되어 있을 때만 재요청을 하도록 의도했어요.
제가 실험했던 환경은 "브라우저에서 코레아 서비스의 알림페이지를 켜두고, 다른 브라우저 탭을 열어서 활동할 때 refetch 해오지 않는다"를 확인했어요! 제 추측으로는 작은 화면으로 켜 놓은 브라우저 창이 인식이 된 거 아닐까? 싶네용
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.
아하 포커싱 안 된 게 인식이 안 된 것 같네요... 다시 실험해보고 또 안 되면 공유드리겠습니다ㅎㅎㅎ (실험 실패하면 좀 재밌어짐)
<ContentSection title="알림 내역"> | ||
<S.AlarmList> | ||
{alarmListData?.map((alarm) => ( | ||
<AlarmItem key={alarm.alarmId} alarm={alarm} onAlarmClick={handleAlarmClick} /> |
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.
AlarmItem 컴포넌트로 분리한 거 좋네요ㅎㅎㅎ💯💯
REVIEW_URGE: (interactionId: number) => `/rooms/${interactionId}`, | ||
MATCH_COMPLETE: (interactionId: number) => `/rooms/${interactionId}`, | ||
MATCH_FAIL: (interactionId: number) => `/rooms/${interactionId}`, | ||
FEEDBACK_CREATED: () => "/feedback", |
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.
피드백 페이지로 넘어가서 받은 피드백을 보여주려고 했던 흐름이 맞나용??
const {
selectedFeedbackType,
handleSelectedFeedbackType,
selectedFeedback,
handleSelectedFeedback,
selectedFeedbackData,
} = useSelectedFeedbackData();
handleSelectedFeedbackType("받은 피드백")
handleSelectedFeedback({방 Id})
그 흐름이면 useSelectedFeedbackData 훅을 추가해야할 것 같아요!
(저런 훅이 있다고 참고용으로 넣어두었어요. 여기서는 handle 함수 두 개만 쓰면 될 것 같습니다!)
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.
알람으로 받아오는 데이터가 한 두개가 아니라서 조금 복잡하고 어려워보이는데 잘 구현한 것 같아요 😄
고생했어요 초코!
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.
테스트 데이터까지 잘 넣어주셨군요 👍
queryFn: getAlarmList, | ||
queryFn: async () => { | ||
const result = await getAlarmList(); | ||
queryClient.invalidateQueries({ queryKey: [QUERY_KEYS.ALARM_COUNT] }); |
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.
queryFn에서 invalidateQueries 를 처리해줬는데 이렇게 만든 이유가 있나요??
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.
아하 알림 리스트를 불러오면서 알림 개수를 다시 최신화 시키기 위해서 작성했군요
올바르지 않다기보다 queryFn 에 invalidateQueries가 있는게 조금 부자연스럽다는 생각을 했었는데 알림 리스트를 불러오면서 알림개수를 같이 최신화시킨다하면 괜찮은거 같네요!
export const EmptyContainer = styled.div` | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
|
||
width: 100%; | ||
height: calc(100vh - 105px); | ||
`; |
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.
꼼꼼하게 봐주셔서 이것까지 수정했습니다~
…ME를 ALARM_MESSAGES로 통일하고 상수파일로 분리
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.
서버가 돌아왔네요ㅎㅎㅎ 피드백 모아보기 페이지는 잘 넘어가는 걸 확인했습니다❗❗
코멘트 남겼는데 확인해주세요~!
if (path) { | ||
navigate(path); | ||
} | ||
handleNavigation(actionType, interaction.interactionId); |
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.
alarmType이 USER 혹은 SERVER 값을 가지는데, SERVER값을 가지는 데이터가 늦게 추가된 거라서 놓친 것 같아요.
타입에 관련없이 항상 USER값으로 넘겨주느라 읽음 표시가 안됐나봐요! 수정했습니다:) 👍
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.
아 USER 때문에 true로 안 바꼈던게 맞았네요!!! 요청 잘 가고 isRead가 true로 바뀌는 거도 확인했습니다!!💯💯
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.
selectedFeedback을 sessionStorage로 저장하는 과정에서 꼬이는 게 있네요.. 제가 작업했던 부분이라 이 PR이 머지되면 바로 어떻게 해결할 지 정해서 수정하고 PR 올리겠습니다!!
서버 오류 전, 후로 나누어서 작업하느라 고생 많으셨어요😭 이렇게 호흡 길게 코드리뷰 한 건 오랜만인 것 같네요! 피드백 다 반영해주셔서 감사합니다. 특히 ALARM_MESSAGES 리팩토링은 제가 생각했던 방향과 똑같아서 좋았습니다👍👍
📓 스토리북 링크
바로가기
📌 관련 이슈
✨ PR 세부 내용
구현 내용
추가 구현
alarm/count
와alarm
api 둘다 1분마다 재요청하여 최대한 실시간처럼 보이도록 구현