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

Feature/#247 팀 rud api 연결 #248

Merged
merged 8 commits into from
Jun 29, 2024

Conversation

pipisebastian
Copy link
Member

@pipisebastian pipisebastian commented Jun 7, 2024

관련 이슈

작업 요약

  • 팀 create부분을 제외한 rud 연결
  • 아직 백엔드 도움이 필요한 이슈로.. api 테스트는 못했습니다!

작업 상세 설명

  • 코멘트 남기겠습니다!

리뷰 요구 사항

  • 8분

미리 보기

image image image

@pipisebastian pipisebastian added the 📬 API 서버 API 통신 label Jun 7, 2024
@pipisebastian pipisebastian self-assigned this Jun 7, 2024
}

export interface TeamDetail extends Team {
attendanceRate: number;
Copy link
Member Author

Choose a reason for hiding this comment

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

TeamDetail은 팀 조회에서만 쓰입니다.
백엔드 엔티티를 보니 attendanceRate는 들어가있지 않고, 이건 백엔드에서 따로 계산해서 주는 값이더라구요. 여쭤보니 팀 조회말고 다른데서 쓰이는 곳이 없을거라 하셔서, Team 타입에 옵셔널로 attendanceRate 을 넣는것보다, 따로 타입을 만들어주는 편이 깔끔하다 생각했습니다!

export interface Team {
  readonly id: number;
  name: string;
  description: string;
  imageUrl: string;
  attendanceRate? number;
} 이것보다요!

Copy link
Member Author

Choose a reason for hiding this comment

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

그리고 TeamDetail 타입명 짓기가 조금 어려웠는데요.. 제가 Team 타입의 데이터를 저장하는 변수를 만들때, team 보다는 teamInfo 로 지었단 말이죠(변수명이 team이면 조금 와닿지 않아서, info를 붙혀줬었습니다) 밑에 코드처럼요!

const teamInfo: Team = {어쩌구 저쩌구}

그래서 보통 teamInfo를 붙혀주니까, 팀 조회에서 쓰여야 할 "팀 정보"의 타입(지금 보이는 TeamDetail)을 어떻게 정해야할지 고민이 되더라구요.. teamWithAttendanceRate 는 너무 긴 것 같고,, 그래서 api 명이 "팀 상세 조회"니까, 상세를 강조하자..! 싶어서 detail를 붙혔습니다..!

더 좋은 방법/네이밍 있으시면 추천부탁드립니닷!!

id: 99,
name: '열사모',
description: '팀 설명이옵니다',
imageUrl: 'https://doo-re-dev-bucket.s3.ap-northeast-2.amazonaws.com/images/005bd2bf-b000-47e2-b092-f5210416ecbc.png',
Copy link
Member Author

Choose a reason for hiding this comment

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

이건 백엔드 이미지 아무거나 가져왔습니다!

<Image w="40" alt="thumbnail" src={thumbnailPath} />
) : (
thumbnail && <Image w="40" alt="thumbnail" src={URL.createObjectURL(thumbnail)} />
)}
Copy link
Member Author

Choose a reason for hiding this comment

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

thumbnailPath은 백엔드에서 가져온 url고, thumbnail은 사용자가 올린 FIle 객체입니다.
맨 처음 수정 모달을 켰을때, 현재 팀 이미지가 보이는게 맞는 것 같아, 있을 경우에 기본으로 뒀고,

  1. 서버 이미지 있다면 => 서버 이미지
  2. 사용자가 이미지 업로드 했다면 => 자기가 올린 이미지

둘 중 1개만 보이도록 했습니다!

@@ -112,6 +152,7 @@ const TeamModal = ({ isOpen, setIsOpen }: TeamModalProps) => {
onChange={(e) => {
if (e.target.files && e.target.files[0]) {
setThumbnail(e.target.files[0]);
setThumbnailPath('');
Copy link
Member Author

Choose a reason for hiding this comment

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

여기서는 만약 사용자가 이미지를 업로드 했다면, 서버 이미지를 그냥 빈 문자열로 바꿔줬습니다. 165 line에 보이는 것처럼, 둘중 하나만 보이도록 하려구요! 근데 이렇게 되면 다시 이미지 되돌리기를 못한다는 단점이 있습니다..

사용자 입장에서는 이미지를 혹시라도 잘못 올리면(api는 안보냈지만, 프론트 상에서 이미지를 올렸을때), 원래 이미지로 못 돌아가고, 취소하기 위해서는 아예 모달창을 껐다 켜야 합니다..

이부분은 썸네일 올리는 부분에 [취소] 버튼을 넣는다거나 해서... 아예 이미지 업로드 공통 컴포넌트를 만드는게 좋아보입니다! (배포 이후...)

onSubButtonClick={onClose}
mainButtonText="저장"
onMainButtonClick={onSave}
onSubButtonClick={resetAndCloseModal}
Copy link
Member Author

Choose a reason for hiding this comment

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

resetAndCloseModal 이름보다는 사실 handleCloseModalButtonClick, handleCancleButtonClick 이런식으로 핸들러 이름으로 짓고 싶었는데요.. (101, 104 line)

state를 초기화하고 modal을 닫는 동작이, 해당 104 line 말고 101, 67, 87 line에 다 들어간단 말이죠..
그런데 101, 67, 87 line 에서 handleCloseModalButtonClick 을 호출한다는게 어색해보여서, 그냥 resetAndCloseModal 으로 지었습니다!

물론

const `handleCloseModalButtonClick`  = () => {resetAndCloseModal();}
const  `handleCancleButtonClick` = () =>  {resetAndCloseModal();}

이렇게 함수 2개 만들면 핸들러 네이밍 붙힐 수 있긴 한데용,, 뭔가 굳이라는 생각이 들었습니다....

const inputFileRef = useRef<HTMLInputElement>(null);
const [name, setName] = useState<string>('');
const [description, setDescription] = useState<string>('');
const [thumbnailPath, setThumbnailPath] = useState<string>('');
Copy link
Member Author

Choose a reason for hiding this comment

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

thumbnailPath는 서버에서 받는 이미지 주소입니다

aria-label="delete-team-button"
onClick={() => setIsDeleteModalOpen(true)}
size="xs"
>
Copy link
Member Author

Choose a reason for hiding this comment

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

요 부분은 스터디코드 슬쩍해와씁니다!


const Page = () => {
// TODO 팀 조회 연결
const teamInfo = teamInfoData;
Copy link
Member Author

Choose a reason for hiding this comment

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

teamInfoData는 현재 mocks데이터입니다! api 연결 필요합니다

@@ -90,7 +90,7 @@ const SidebarContent = ({ isOpen, setIsOpen }: SidebarContentProps) => {
</>
)}
</Flex>
<TeamModal isOpen={isTeamModalOpen} setIsOpen={setIsTeamModalOpen} />
<TeamModal isOpen={isTeamModalOpen} onClose={() => setIsTeamModalOpen(false)} />
Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분 제맘대로 슬쩍 바꿨는데요! 사실 모달창에서 모달을 닫는 작업만 필요하기 때문에, setter로 넘겨줘야하나 싶었습니다! 그래서 모달창 닫는 함수로 넘겨줬습니다!

@@ -11,7 +11,7 @@ const Title = ({ isTeam = false, teamImg, name, description }: TitleProps) => {
borderColor="gray.100"
shadow="none"
size="md"
src={teamImg || '/images/doore_logo.png'}
src={imageUrl ?? '/images/doore_logo.png'}
Copy link
Member Author

Choose a reason for hiding this comment

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

여기는 null 병합 연산자(??)로 넣어줬습니다! || 는 Falsy값이 다 들어가기 때문에, 확실하게 undefined/null만 판단하도록 ?? 로 바꿨습니다!

그런데 백엔드에 여쭤보니까, 이미지 아무것도 안넣으면, 백엔드에서 기본 디폴트 이미지 url을 던져준다고 하더라구요! 그래서 사실 ?? '/images/doore_logo.png' 로 프론트상에서 디폴트 이미지 지정해주는게 필요없어지긴 합니다!

그런데 현재는 이미지 아무것도 안넣었을때, null을 던져준다고 합니다!(추후 디폴트 url로 바꿀 예정이래요)

Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 api 연결 오류로 undefined 일 수 있으니 이후에도 계속 유지해도 될듯 합니다!

@@ -10,10 +10,10 @@ const postCreateTeam = (team: FormData) =>
body: team,
});

const putEditTeam = (teamId: number, team: EditTeamDto) =>
const putEditTeam = (teamId: number, teamInfo: Pick<Team, 'name' | 'description'>) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

아하! 이런 식으로 Pick 해서 쓰는 거군요! 새로 배워갑니다🤩

Comment on lines 1 to 4
import { Team } from '@/types';

export interface TitleProps extends Omit<Team, 'id'> {
isTeam?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 헷갈려서 그런데, 스터디 페이지에선 이미지를 안 쓰지 않나요?
이렇게 되면 스터디 페이지에서 Title을 사용할 때 imageUrl을 어떻게 처리해야 할지 모르겠네요..!

그리고 Title은 공통 컴포넌트라서 꼭 Team을 상속받아야 하는지 잘 모르겠어요!

Copy link
Member Author

@pipisebastian pipisebastian Jun 8, 2024

Choose a reason for hiding this comment

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

음음 그렇네용..! Team에서 빼먹을 수 있으니까 Team을 가져온건데, 팀 + 스터디 둘다 쓰이니까, 조금 애매해보이네욥..!
그리고 말씀하신대로 imageUrl은 study에 안쓰여서, 옵셔널 처리하겠습니당!!

아예 타입을 따로 만드는게 더 좋을까요?? 원래대로 도르마무!!

export interface TitleProps {
  isTeam?: boolean;
  name: string;
  description: string;
  imageUrl?: string;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

캬햐 사실 방금 디벨롭 풀받으면서 충돌 해결하다가,,, 이 코드로 푸시를 해버려씁니다..!
더 좋은 방법있으시면 말해주세용!! 바로 수정할게요!

Comment on lines +44 to +51
const isTeamInfoValid = () => {
const isValidName = name.trim() !== '';
const isValidDescription = description.trim() !== '';
setAlertName(!isValidName);
setAlertDescription(!isValidDescription);

return isValidName && isValidDescription;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

함수 분리하니까 예쁘네요👍
스터디 모달도 참고해야겠어요~!

mainButtonText="저장"
onMainButtonClick={onSave}
onSubButtonClick={resetAndCloseModal}
mainButtonText={teamInfo ? '수정' : '생성'}
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
Collaborator

@jasper200207 jasper200207 left a comment

Choose a reason for hiding this comment

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

👍

@@ -11,7 +11,7 @@ const Title = ({ isTeam = false, teamImg, name, description }: TitleProps) => {
borderColor="gray.100"
shadow="none"
size="md"
src={teamImg || '/images/doore_logo.png'}
src={imageUrl ?? '/images/doore_logo.png'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 api 연결 오류로 undefined 일 수 있으니 이후에도 계속 유지해도 될듯 합니다!

@pipisebastian pipisebastian requested a review from yeonddori June 8, 2024 10:29
…into feature/#247-팀_RUD_API_연결

# Conflicts:
#	src/app/team/[teamId]/page.tsx
Copy link
Collaborator

@llddang llddang left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

@pipisebastian pipisebastian merged commit 4500e99 into develop Jun 29, 2024
1 check passed
@pipisebastian pipisebastian deleted the feature/#247-팀_RUD_API_연결 branch June 29, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📬 API 서버 API 통신
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants