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

SKRF-141 design: 아바타 공통 컴포넌트 구현-리액트 아이콘 미사용 #19

Merged
merged 9 commits into from
Oct 25, 2023

Conversation

colorkite10
Copy link
Collaborator

@colorkite10 colorkite10 commented Oct 25, 2023

📝요구사항과 구현내용

  • 아바타 공통 컴포넌트를 구현했습니다.
    avatarShape: 'rectangle' | 'normal' | 'large' | 'small' | 'medium'

rectangle: 사이드바 유저 프로필
normal: 제일 많이 쓰이는 크기
large: 클럽 생성 혹은 유저 프로필 사진 업로드 시, isEdit:true와 함께 쓰임
medium: 클럽 프로필

구현 스크린샷

image

✨pr포인트 & 궁금한 점

  • 피그마 기준으로 만들긴 했는데 생각보다 이미지 사이즈들이 다 커서 조정할 필요가 있어 보입니다.
  • 새 리팩토링 브랜치로 리액트아이콘 적용하겠습니다! 리팩토링 적용 후 컴포넌트 사용법도 주석으로 달아 놓을까요?
  • @SongInjae 충돌이 InputForm 컴포넌트끼리 난 것 같은데 혹시 충돌 해결 가능하실까요? 🙋‍♀️

@colorkite10 colorkite10 self-assigned this Oct 25, 2023
Copy link
Collaborator

@hyesung99 hyesung99 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 채연님! 리뷰에대한 질문있으시면 언제든 말해주세요b
주석은 지양하기로 컨벤션에서 정했으니까 최대한 코드로 표현할 수 있게 만들면 좋을것 같아요!

Comment on lines 29 to 36
const ProfileImageStyled = styled.img<AvatarProps>`
position: relative;
width: ${({ avatarShape }) => getAvatarSize(avatarShape)};
height: ${({ avatarShape }) => getAvatarSize(avatarShape)};
border-radius: ${({ avatarShape }) => (avatarShape === 'rectangle' ? '1.7rem' : '50%')};
object-fit: cover;
cursor: pointer;
`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

벌써 이모션 사용법을 익히셨군요!b

Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
cursor: pointer는 모든 아바타가 클릭 가능하기 때문일까요??
제 생각엔 클릭 못하는 아바타도 있을것 같아요! ex)프로필페이지에서 아바타

Copy link
Collaborator

Choose a reason for hiding this comment

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

@colorkite10 +추가의견
그래서 아바타가 cursor를 결정하기보다는
children으로 프로필로 옮겨갈 수 있는 Link컴포넌트를 받는건 어떨까요?
그 Link컴포넌트에 cursor를 설정하는거죠!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

우선 추후 접수임니다!

}
};

const Avatar = ({ avatarShape, profileImage, isEdit, onClick }: AvatarProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;
profileImage보다는 좀 더 명확하게 profileImageSrc는 어떨까요?

Comment on lines 10 to 25
const getAvatarSize = (avatarShape: AvatarProps['avatarShape']) => {
switch (avatarShape) {
case 'small':
return '1.5rem';
case 'normal':
return '4rem';
case 'rectangle':
return '4.5rem';
case 'medium':
return '6.875rem';
case 'large':
return '16.3rem';
default:
return '4rem';
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

p2;
getAvatarSize가 두 파일에 각각 선언되어 있네요! 컴포넌트 외부에 함수를 선언하기보다는 uitils폴더에 만들면 코드 가독성이 더 좋아질 것 같아요! 모듈화하면 다른 컴포넌트에서도 재사용할 수 있다는 장점이 있구요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

utils폴더에 만드려다가 utils내부 폴더 구조를 어떻게 작성할지 고민되어서 안 해버렸네요... 하하! util함수들을 어떻게 묶어서 폴더로 만들까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 utils는 여러 도메인에서 사용할 수 있을것 같아서 폴더구조 생성안하고 그냥 파일로했어요!

Comment on lines 28 to 29
const width = getAvatarSize(avatarShape);
const height = getAvatarSize(avatarShape);
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;

const width = height = getAvatarSize(avatarShape);

이렇게 표현할수도있어요! 함수호출을 한번만 해도 되는거죠..!

Copy link
Collaborator

Choose a reason for hiding this comment

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

p2;
그리고 style파일 안에서 이미 avatarShape을 기준으로 width와 height를 계산하고 있어서 여기서 한번 더 하는 이유가 없을듯 하네요...!
제가 잘못이해했다면 댓글남겨주세요^.^!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

여기서 받는 이유는 리액트 아이콘의 기본이미지 아이콘 크기를 설정해주기 위함이었습니다! 이 부분은 라이브러리 이용 후 확인해보겠습니다

import { AvatarContainerStyled, EditButtonStyled, ProfileImageStyled } from './Avatar.style';

interface AvatarProps {
avatarShape: 'rectangle' | 'normal' | 'large' | 'small' | 'medium';
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
avatarType은 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

뭔가 타입스크립트의 타입이랑 헷갈릴 것 같아서 shape으로 지정했습니다!

Comment on lines 34 to 49
{profileImage ? (
<ProfileImageStyled
avatarShape={avatarShape}
src={profileImage}
alt="profile image"
style={{ width: `${width}`, height: `${width}` }}
/>
) : (
<img
src="https://picsum.photos/200/300"
width={width}
height={height}
style={{ width: `${width}`, height: `${width}` }}
alt="리액트 아이콘 기본이미지로 바꿀 부분"
/>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{profileImage ? (
<ProfileImageStyled
avatarShape={avatarShape}
src={profileImage}
alt="profile image"
style={{ width: `${width}`, height: `${width}` }}
/>
) : (
<img
src="https://picsum.photos/200/300"
width={width}
height={height}
style={{ width: `${width}`, height: `${width}` }}
alt="리액트 아이콘 기본이미지로 바꿀 부분"
/>
)}
<ProfileImageStyled
avatarShape={avatarShape}
src={profileImage ? profileImage : '기본이미지'}
alt="profile image"
/>

이렇게 줄일수 있지 않을까요?

interface AvatarProps {
avatarShape: 'rectangle' | 'normal' | 'large' | 'small' | 'medium';
profileImage?: string;
isEdit?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p5;
isEditable은 어떨까요?

Comment on lines 6 to 11
interface AvatarProps {
avatarShape: AvatarShapeType['avatarShape'];
profileImageSrc?: string;
isEditable?: boolean;
onClick?: () => void;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@colorkite10 p1;
채연님도 onClick에 대한 타입 정의때문에 고민 많이 하셨을 것 같아요!

Suggested change
interface AvatarProps {
avatarShape: AvatarShapeType['avatarShape'];
profileImageSrc?: string;
isEditable?: boolean;
onClick?: () => void;
}
interface AvatarProps extends React.HTMLAttributes<HTMLDivElement> {
avatarShape: AvatarShapeType['avatarShape'];
profileImageSrc?: string;
isEditable?: boolean;
}

이렇게 하면 onClick의 타입을 HTMLDivElement에서 상속받아 사용할 수 있습니다!

1.HTMLDivElement 설명 https://developer.mozilla.org/ko/docs/Web/API/HTMLDivElement
2.HTMLDIvElement에 선언된 타입들
https://microsoft.github.io/PowerBI-JavaScript/interfaces/_node_modules_typedoc_node_modules_typescript_lib_lib_dom_d_.htmldivelement.html#onclick

Copy link
Collaborator

Choose a reason for hiding this comment

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

오옹~ 저도 활용해야겠군요! 꿀팁 감사합니다

SongInjae
SongInjae previously approved these changes Oct 25, 2023
Copy link
Collaborator

@SongInjae SongInjae 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 6 to 11
interface AvatarProps {
avatarShape: AvatarShapeType['avatarShape'];
profileImageSrc?: string;
isEditable?: boolean;
onClick?: () => void;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오옹~ 저도 활용해야겠군요! 꿀팁 감사합니다

hyesung99
hyesung99 previously approved these changes Oct 25, 2023
Copy link
Collaborator

@SongInjae SongInjae left a comment

Choose a reason for hiding this comment

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

고생하셨습니다:)

Copy link
Collaborator

@hyesung99 hyesung99 left a comment

Choose a reason for hiding this comment

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

찐승인

@colorkite10 colorkite10 merged commit 94f78c6 into main Oct 25, 2023
2 checks passed
@colorkite10 colorkite10 deleted the SKRF-141-design-avatar-component branch October 25, 2023 07:39
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.

3 participants