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

[Fix] 초대 페이지 및 대시보드 페이지 QA #499

Merged
merged 8 commits into from
Mar 6, 2025

Conversation

Bowoon1216
Copy link
Contributor

해당 이슈 번호

closed #498


체크리스트

  • 🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
  • 💯 테스트는 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 불필요한 코드는 제거했나요?
  • ✅ 컨벤션을 지켰나요?
  • 💭 이슈는 등록했나요?
  • 🏷️ 라벨은 등록했나요?
  • 💻 git rebase를 사용했나요?
  • 🙇‍♂️ 리뷰어를 지정했나요?

💎 PR Point

  • 초대 열람 에러 처리
  • 워크스페이스 삭제시 헤더 널 이슈 해결
  • 코드 정리 및 팀용량 단위 수정

📌스크린샷 (선택)

Copy link

github-actions bot commented Mar 4, 2025

🚀 Storybook 확인하기 🚀

Copy link

github-actions bot commented Mar 4, 2025

🚀 Storybook 확인하기 🚀

Copy link

github-actions bot commented Mar 4, 2025

🚀 Storybook 확인하기 🚀

Copy link
Contributor

@rtttr1 rtttr1 left a comment

Choose a reason for hiding this comment

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

LGTM~~ 드디어 QA가 끝나가네요!! 고생하셨습니다!

Copy link
Member

@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.

고생하셨습니다 ~! 몇 개의 코멘트만 확인해주세용

}
}, [createToast, data, failureCount, invitationId, invitationInfo?.teamId, isLogined, navigate]);
// eslint-disable-next-line react-hooks/exhaustive-deps
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.

useEffect안에서 handleApiError 함수를 호출했는데 이 함수도 의존성배열로 취급해버려 함수는 랜더링될때마다 새로 생성되고 useEffect도 매번 생성되는 경고가 떠서 저걸로 막았습니다.
근데 이제 생각해보니 저 함수는 useEffect안에서만 사용이 되므로 안에서 생성하면 의존성배열에 추가하지 않아도 되더라구요!? 그래서 안으로 모셨습니다.~

Comment on lines +74 to +82
onSuccess: (res) => {
if (res.success) {
createToast(res.message, 'success');

localStorage.setItem(STORAGE_KEY.TEAM_ID, `${teamId}`);
localStorage.setItem(STORAGE_KEY.TEAM_NAME, `${invitationInfo?.teamName}`);
} else {
createToast(res.message, 'error');
}
Copy link
Member

Choose a reason for hiding this comment

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

왜 이렇게 하신거죠 ? onSuccess가 이미 쿼리로 데이터를 가져오는데 성공했을 때 콜백이 실행하므로 재검증하는 로직은 불필요할 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

초대 수락에 실패해도 onSuccess로 들어와서 res.success 확인을 하지 않으면 '이미 존재하는 회원입니다'와 같은 에러 메시지도 success toast로 뜹니다.
제 생각엔 api 결과가 모두 { success: boolean, message: string } 객체로 반환되어 실패하든 성공하든 onSuccess함수로 들어오게 되는 것 같습니다. 그래서 onSuccess 안에서 다시 구분해줬습니다.

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

github-actions bot commented Mar 5, 2025

🚀 Storybook 확인하기 🚀

Copy link
Member

@namdaeun namdaeun 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 +74 to +82
onSuccess: (res) => {
if (res.success) {
createToast(res.message, 'success');

localStorage.setItem(STORAGE_KEY.TEAM_ID, `${teamId}`);
localStorage.setItem(STORAGE_KEY.TEAM_NAME, `${invitationInfo?.teamName}`);
} else {
createToast(res.message, 'error');
}
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 +105 to +110
onSuccess: (res) => {
if (res.success) {
createToast(res.message, 'success');
} else {
createToast(res.message, 'error');
}
Copy link
Member

Choose a reason for hiding this comment

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

요것도 같은 문제인가보네요 ,,

@Bowoon1216 Bowoon1216 merged commit 36e4628 into develop Mar 6, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

초대 QA
4 participants