Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the UI and API for displaying group study participants. The main additions include a new member item component that shows participant profiles, greetings, and study progress with expandable history, along with the corresponding API integration for fetching member lists.
Key Changes
- Added a reusable date formatting enhancement to support both dash and dot separators
- Created comprehensive UI component for displaying group study member information including profile, greeting, and progress tracking
- Implemented API layer for fetching paginated group study member lists
Reviewed Changes
Copilot reviewed 6 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/lib/time.ts | Enhanced date formatter to support dot separator in addition to dash |
| src/features/study/group/ui/group-study-member-item.tsx | New component displaying member profile, greeting, and progress with expandable history |
| src/features/study/group/model/use-group-study-member-list-query.ts | React Query hook for fetching group study member list |
| src/features/study/group/api/group-study-types.ts | Type definitions for group study member API responses |
| src/features/study/group/api/get-group-study-member-list.ts | API function to fetch group study member list with optional pagination |
| src/features/admin/ui/member-list-table.tsx | Updated mentor icon styling with explicit dimensions and color |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| {/* 사용자 프로필 */} | ||
| <div className="relative inline-block"> | ||
| <Image | ||
| src={member.profileImageUrl} |
There was a problem hiding this comment.
The profileImageUrl can be null according to the type definition. This will cause Next.js Image component to fail. Add a fallback image or conditional rendering to handle null values.
| src={member.profileImageUrl} | |
| src={member.profileImageUrl || '/images/default-profile.png'} |
| pageNumber, | ||
| pageSize, | ||
| }: GroupStudyMembersRequest): Promise<GroupStudyMembersResponse> => { | ||
| const isPaging = pageNumber && pageSize ? true : false; |
There was a problem hiding this comment.
This can be simplified to const isPaging = !!(pageNumber && pageSize); or just use the boolean expression directly without the ternary operator.
| const isPaging = pageNumber && pageSize ? true : false; | |
| const isPaging = !!(pageNumber && pageSize); |
| const [isProgressHistoryOpen, setIsProgressHistoryOpen] = | ||
| useState<boolean>(false); | ||
|
|
||
| const isMe = member.id === Number(getCookie('memberId')); |
There was a problem hiding this comment.
The isMe check is duplicated in both the main component (line 27) and the GreetingBox component (line 205). Consider passing isMe as a prop to GreetingBox to avoid redundant cookie access and computation.
| <div className="bg-fill-warning-default-default rounded-50 p-150"> | ||
| <SealCheckIcon className="text-text-inverse" width={24} height={24} /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The switch statement doesn't handle the 'F' grade case that is defined in the Grade type. This will return undefined for 'F' grades, causing a rendering issue. Add a case for 'F' grade or a default case.
| ); | |
| ); | |
| case 'F': | |
| return ( | |
| <div className="bg-fill-danger-default-default rounded-50 p-150"> | |
| <SealCheckIcon className="text-text-inverse" width={24} height={24} /> | |
| </div> | |
| ); | |
| default: | |
| return null; |
There was a problem hiding this comment.
UI를 분리하지 않고, 한 컴포넌트 내에 다 넣으신 이유가 있으신가요? 유지, 보수 측면에서 여러 함수를 한 컴포넌트 안에 다 몰아넣는 것은 유지 보수 측면에서 권장 되지 않습니다. 왜냐하면, 파일 길이 증가, 재사용성 저하, 관심사 분리 어려움 등이 이유가 있을 수도 있는데요. 가능하다면 특별한 이유가 없다면, 분리하는 것을 권장드립니다.
추가로 github copilot 자동리뷰 추가하였습니다. 확인하시고, copilot에 제시한 내용이 괜찮다고 생각하시면, 수정하시고 copilot이 준 피드백이 별로다 하면 resolve conversation 눌러서 무시하시면 될듯 싶습니다.
There was a problem hiding this comment.
이 폴더 안에 컴포넌트들을 몰아넣은 이유는 이 UI에서만 사용되는 컴포넌트들이라 한 곳에 몰아두었습니다.
현재 다른 곳에서 재사용할 것 같지 않아서 한 곳에 몰아두었습니다.
파일 길이 증가, 재사용성 저하, 관심사 분리 어려움
충분히 분리할만한 이유라고 생각합니다. 아직 한 파일에 하나의 컴포넌트를 두자는 컨벤션이 없기도 하고, ui 폴더에 컴포넌트 파일이 많이 쌓일 것 같아서 우선 한 곳에 몰아넣었습니다.
🌱 연관된 이슈
571
572
☘️ 작업 내용
그룹 스터디 참여자 항목 UI 만들었습니다. (더보기 버튼은 아직 구현하지 않았습니다. 다른 PR에서 구현할 예정입니다.)
본인이고 가입인사를 작성하지 않으면 아래와 같은 UI를 보여줍니다. (작성하기 버튼만 있고, 이 기능은 다른 PR에서 작업할 예정입니다.)
본인이 아니고 가입인사를 작성하지 않으면 아래와 같은 UI를 보여줍니다.