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 minor issues for executive activity #1394

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

wjeongchoi
Copy link
Contributor

@wjeongchoi wjeongchoi commented Jan 27, 2025

요약 *

It closes #1388

  • editedAt, commentedAt 사용
  • 승인 코멘트만 있을 때 반려사유 필드 제거
  • 승인 코멘트 보이는 부분 제거
  • 승인/반려 시 table 최신화
  • 검토 현황 상세 rowLink 추가
  • 집행부 상세 페이지에서 pagehead가 대표 동아리 관리로 뜨는 문제

스크린샷

이후 Task *

  • 없음

@wjeongchoi wjeongchoi added bug Something isn't working front-end labels Jan 27, 2025
@wjeongchoi wjeongchoi self-assigned this Jan 27, 2025
@wjeongchoi wjeongchoi linked an issue Jan 27, 2025 that may be closed by this pull request
@wjeongchoi wjeongchoi marked this pull request as ready for review January 27, 2025 15:34
@@ -248,30 +254,28 @@ const ActivityReportDetailFrame: React.FC<ActivityReportDetailFrameProps> = ({
labels={
getActivityReportProgress(
data.activityStatusEnumId,
data.updatedAt,
data.activityStatusEnumId === 1
Copy link
Contributor

Choose a reason for hiding this comment

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

요런거 숫자 말고 인터페이스에 ActivityStatusEnum 사용하면 좋을 것 같아요

@@ -64,6 +65,7 @@ const useGetActivityReportDetail = (

return {
data: memoizedData,
clubId: activityReport?.clubId ?? 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 요거 memoizedData 에 있는 clubId 그대로 사용하지 않고 따로 뺀 이유가 있나요?
그렇다면 ActivityReportDetailFrame 199 번째 줄의 !("clubId" in data) 이라는 조건을 !clubId 혹은 그냥 아예 지워도 될거 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

정확한 이유는 모르겠지만 clubId: memoizedData.clubId 이렇게 쓰면 아래 같은 에러가 나요
스크린샷 2025-02-02 오후 11 29 40
!("clubId" in data) 대신 clubId === 0로 바꿨는데 괜찮을까요

@@ -226,6 +228,10 @@ const ActivityReportDetailFrame: React.FC<ActivityReportDetailFrameProps> = ({
return null;
};

const filteredComments = data.comments.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

별거 아니긴 한데 요거 수정페이지 쪽에서도 똑같이 필터하고 있는 것 같고 활동이 승인되었습니다 라는 상수 스트링도 사용하고 있어서 함수로 밖으로 빼는건 어떨까?

data.updatedAt,
data.activityStatusEnumId === 1
? data.editedAt
: data.commentedAt || data.editedAt,
Copy link
Contributor

Choose a reason for hiding this comment

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

홈 뭔가 가독성이 조굼 떨어지는거 같은데 그냥 commentedAt이 있으면 그걸로 없으면 editedAt으로 하면 안되나?
commentedAt 이 있는데 activityStatusEnum이 applied 일 수가 있나?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이미 한 번 검토가 됐는데 다시 수정하면 applied로 바뀌어서 상태가 applied일 때만 editedAt을 보여줘야 하고,
commentedAt은 null이 가능해서 없을 경우에 사용할게 필요해서 updatedAt으로 수정했어

@@ -18,6 +22,12 @@ const useExecutiveApproveActivityReport = (activityId: number) => {
queryClient.invalidateQueries({
queryKey: activityReportDetailQueryKey("executive", activityId),
});
queryClient.invalidateQueries({
queryKey: ["executiveChargedActivities"],
Copy link
Contributor

Choose a reason for hiding this comment

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

요거 딱히 쿼리키 정의된 곳은 없이 사용만 하고 있는거 같은데 요거 뭔가요?

Copy link
Contributor Author

@wjeongchoi wjeongchoi Feb 2, 2025

Choose a reason for hiding this comment

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

이거 #1367 에서 써용

Copy link
Contributor

Choose a reason for hiding this comment

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

그 pr에서 쿼리키 수정한 것 같은데 여기도 하는거죠?

cell: info => formatDateTime(info.getValue()),
cell: info => {
const date = info.getValue();
return date ? formatDateTime(date) : "-";
Copy link
Contributor

Choose a reason for hiding this comment

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

요거 지금 스테이지라서 요런거겠지? 상태가 신청 상태인데 commentedAt이 있어서 "-" 가 안 나와! 프론트에선 상태로 따로 더 검증 안해도 되려나?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

물어봤는데 commentedAt 오면은 보여주는게 맞대요

@jooyeongmee
Copy link
Contributor

혹시 그때 슬랙에서 논의 한 최종 승인 상태에 따라 토스트 색상 바꾸는거 요기 말고 다른 곳에서 하나요?
image

@jooyeongmee
Copy link
Contributor

칼디 임원진 회의 활보 (1725번) 상세조회 하면 이런 에러가 납니다 확인 부탁해요
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix minor issues for executive activity
2 participants