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

Tool setting & 공통교양 영역 내 채플 카테고리 추가 #150

Merged
merged 14 commits into from
Oct 3, 2024

Conversation

yougyung
Copy link
Member

@yougyung yougyung commented Sep 24, 2024

📌 작업 내용

구현 내용 및 작업 했던 내역

  • cs 도구 channel-talk 설정
  • 모니터링 도구 google-analytics 설정
  • 누락된 아이디 찾기 페이지, 비밀번호 재설정 페이지의 metadata 추가
  • 결과페이지 공통교양 영역내에 채플카테고리 및 내역을 생성하여 추가
    • 기존 안내 문구를 제거하고 채플 카테고리를 추가 생성해 직관적 이해가 가능하도록 변경했어요
    • 기존 api에서 전달받고 있지 않으므로 이수학점조회 api를 통해 받은 채플 정보를 기반으로 데이터를 가공했습니다.

🤔 고민 했던 부분

  • 채널톡 컴포넌트를 어디 위치시킬지에 대해 고민했는데, utils의 provider내에 위치한 파일과 성격이 비슷하다는 생각이 들어 추가로 폴더를 생성하지않고 provider -> global로 변경 후 해당 폴더의 하위에 위치시켰어요
  • 최상위 layout파일에 <UserDeleteModal />이 위치하는 것을 확인했습니다. UserDeleteModal이 최상위 layout에 위치하는 것 보다는 아래 두가지 위치 중 하나에 위치하는 것이 의미적으로 더 가까울 것 같다는 생각이 들어 1번으로 변경을 진행해봤는데, 팀원분들의 의견은 어떤지 궁금합니다 !
    1. UserDeleteModalUserDeleteButton이 같은 목적을 갖고 있는 도메인 컴포넌트이므로 UserDeleteModal을 UserDeleteButton에 위치
    2. <UserDeleteModal />이 호출되는 mypage와 navigationBar에 모두 위치

🔊공유

  • firebase의 경우 오염을 막기 위해 env.production을 생성하였으며, 카톡을 통해 공유해드리겠습니다.
  • 테스트 과정에서 확인된 부분인데 마이페이지의 과목 검색에서 채플의 학점 표기에 오차가 있고 채플 과목의 추가가 안되는 문제를 경험했어요. 중복이수가 가능하다는 특수성이 있는 반면, 채플과목의 코드번호가 고유해서 생기는 문제 같네요. 예외처리가 필요할 것 같아요 !

@yougyung yougyung self-assigned this Sep 24, 2024
Copy link

Copy link

@yougyung yougyung changed the title Tool setting/#148 Tool setting Sep 24, 2024
@yougyung yougyung changed the title Tool setting Tool setting & 공통교양 영역 내 채플 카테고리 추가 Sep 24, 2024
Copy link
Collaborator

@seonghunYang seonghunYang left a comment

Choose a reason for hiding this comment

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

  • 수고하셨습니다
  • 저는 UserDeleteModal과 UserDeleteButton이 결합될 필요는 없다고 생각해요. UserDeleteModal은 UserDeleteButton 외에도 다른 컴포넌트에 의해 호출될 수 있고, UserDeleteButton도 자신의 역할을 수행하는 데 Modal의 존재를 알 필요가 없기 때문입니다. 둘은 응집되는 것보다 약하게 결합된 상태로 있는 게 더 유연성을 가질 수 있을 것 같아요.
  • 다만 말씀해주신 대로 layout에 위치하는 건 불필요한 렌더링이 발생하니, 해당 모달이 등장하는 페이지 수준에서 종속시키는 게 좋을 것 같습니다.

Comment on lines 5 to 10
if (typeof window !== 'undefined') {
ChannelService.loadScript();
ChannelService.boot({
pluginKey: process.env.NEXT_PUBLIC_CHANNELTALK_PLUGIN ?? '',
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 컴포넌트 내부에 있으면 채널톡 내부 SDK에서 처리해주지 않는 한 리렌더링으로 인한 중복 호출이 발생할 수 있을 것 같은데, 공식 가이드에서 이 위치에 삽입하라고 했던 걸까요?

Copy link
Member Author

@yougyung yougyung Sep 26, 2024

Choose a reason for hiding this comment

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

공식가이드 내에서 위치를 권장하지는 않았고 제가 해당 위치에 선언한 이유는 아래와 같습니다.

  1. 전역에 존재해야할 수 있는 layout
  2. 브라우저 내에서 실행되어야하므로 client component

useEffect를 통해서 리렌더링을 통한 중복호출을 방지할 수 있을 것 같은데, 혹시 더 좋은 방법 제안해주시면 반영해서 수정하겠습니다 ! decf073

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 이해했습니다! 말씀하신 대로 useEffect가 더 적절할 것 같습니다. 현재 방식대로 하면 페이지마다 채널톡을 호출하게 되는데, 이것이 의도하신 바라면 그대로 두셔도 됩니다. 다만, 그렇지 않다면 공식 문서에서 제안하는 대로 최상위 변수를 사용해 한 번만 실행하게 하는 것도 좋아 보입니다.
    https://ko.react.dev/learn/you-might-not-need-an-effect#initializing-the-application

Copy link
Member Author

Choose a reason for hiding this comment

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

7748eaf
주신 의견을 통해서 책임이 더욱 잘 드러나도록 변경해봤습니다. 의견 감사합니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@gahyuun gahyuun left a comment

Choose a reason for hiding this comment

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

고생하셨ㅂ습니다 마이페이지 오류는 작업하겠습니다 알려주셔서 감사합니다~!

app/layout.tsx Outdated
Comment on lines 47 to 48
<GoogleAnalytics />
<ChannelTalk />
Copy link
Member

Choose a reason for hiding this comment

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

ChannelTalk 컴포넌트는 utils에 존재하고 google analytics는 외부에 존재하는 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

커밋 관리 과정에서 누락된 부분 같아요 감사합니다! 28ceccf

Copy link
Member

Choose a reason for hiding this comment

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

수정 감사합니다 고생하셨습니다!
그런데 채플이 1.5인데 3번 보이지 않는건 의도하신걸까요?? 또한 채플을 모두 이수했을 경우에는 채플 테이블이 따로 없는데 이것 또한 의도하신걸까요??
image
image

Copy link
Member Author

@yougyung yougyung Sep 26, 2024

Choose a reason for hiding this comment

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

정확한 화면이 제공되지 않아서 제가 추측해보자면

채플이 1.5인데 3번 보이지 않는 문제

option이 미이수 과목상태에 존재하는 것 같습니다. 기이수 과목상태로 토글을 변경시키는 경우에는 채플과목이 3번 노출될 것이라고 생각합니다.

아마 우측 상단에서 현재 이수학점/총 이수학점을 제공하고 있어서 문제라고 예상하신 것 같습니다. 남은 학점이 아닌 현재 이수학점으로 고정시켜둔 이유는 아래와 같습니다.

  1. 토글은 과목표시를 결정하므로 카테고리가 미이수 상태에 의존하지않음
  2. 졸업사정결과를 목적으로하는 유저들이 궁금한 점은 남은 과목 & 현재 진행사항이라고 파악
    • 현재 이수학점의 노출을 통한 진척도 공유
    • 미이수 상태를 default로하여 충족하지 못한 카테고리 공유

이 부분은 개발하면서도 고민했던 부분인데, 혹시 의견 공유해주시면 더 고민해보겠습니다 !

채플을 모두 이수했을 경우에는 채플 테이블이 따로 없는 문제

화면 캡쳐를 통해 보여주신 화면이 핵심교양영역인데, 채플은 공통교양 소속 카테고리라서 공통교양영역에서 확인 가능하실 것으로 예상됩니다

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 31 to 33
completed: takenChapel * 0.5 >= 2.0,
totalCredit: 2.0,
takenCredit: takenChapel * 0.5,
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
Member Author

Choose a reason for hiding this comment

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

Copy link

Copy link

Copy link
Member Author

@yougyung yougyung left a comment

Choose a reason for hiding this comment

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

다만 말씀해주신 대로 layout에 위치하는 건 불필요한 렌더링이 발생하니, 해당 모달이 등장하는 페이지 수준에서 종속시키는 게 좋을 것 같습니다.

의견 감사합니다! 반영해서 수정했습니다. 2b2b75e 한가지 궁금한 점은 모달의 상태가 layout에 영향을 주지않으므로 리랜더링을 발생시키지 않는다고 생각하는데, 위와같이 생각하신 이유를 설명해주실 수 있나요?

Comment on lines 5 to 10
if (typeof window !== 'undefined') {
ChannelService.loadScript();
ChannelService.boot({
pluginKey: process.env.NEXT_PUBLIC_CHANNELTALK_PLUGIN ?? '',
});
}
Copy link
Member Author

@yougyung yougyung Sep 26, 2024

Choose a reason for hiding this comment

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

공식가이드 내에서 위치를 권장하지는 않았고 제가 해당 위치에 선언한 이유는 아래와 같습니다.

  1. 전역에 존재해야할 수 있는 layout
  2. 브라우저 내에서 실행되어야하므로 client component

useEffect를 통해서 리렌더링을 통한 중복호출을 방지할 수 있을 것 같은데, 혹시 더 좋은 방법 제안해주시면 반영해서 수정하겠습니다 ! decf073

Copy link

Copy link

@seonghunYang
Copy link
Collaborator

다만 말씀해주신 대로 layout에 위치하는 건 불필요한 렌더링이 발생하니, 해당 모달이 등장하는 페이지 수준에서 종속시키는 게 좋을 것 같습니다.

의견 감사합니다! 반영해서 수정했습니다. 2b2b75e 한가지 궁금한 점은 모달의 상태가 layout에 영향을 주지않으므로 리랜더링을 발생시키지 않는다고 생각하는데, 위와같이 생각하신 이유를 설명해주실 수 있나요?

  • 아하, 제가 렌더링이라고 표현한 것은 해당 컴포넌트가 리렌더링을 발생시킨다는 의미는 아니고, 사용하지 않은 페이지에도 컴포넌트가 포함되어 있어 React의 렌더링 범위에 포함된다는 뜻이었습니다. 물론 사소한 문제일 수 있지만, 논리적으로 불필요한 부분이기도 하니 이를 구분하는 것이 더 좋다고 생각합니다.

seonghunYang
seonghunYang previously approved these changes Sep 28, 2024
Copy link

github-actions bot commented Oct 1, 2024

@yougyung yougyung merged commit ac8b910 into main Oct 3, 2024
2 of 3 checks passed
@yougyung yougyung deleted the tool-setting/#148 branch October 3, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GA, CS tool setting & 공통교양영역 내 채플 카테고리 추가
3 participants