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

[Feat] 다른 유저 프로필 보기 #160

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

minjeong9919
Copy link
Contributor

🚀 작업 내용

  • 다른 유저 프로필 hover 시 자세하기 보기 기능 구현
  • 비회원일 경우 회원 기능 접근 시 막도록 설정
  • data null 처리

📝 참고 사항

🖼️ 스크린샷

image

🚨 관련 이슈 (이슈 번호)

✅ 체크리스트

  • Code Review 요청
  • Label 설정
  • PR 제목 규칙에 맞는지 확인

@minjeong9919 minjeong9919 added the ✨ Component 기능 개발 label Jul 15, 2024
@minjeong9919 minjeong9919 self-assigned this Jul 15, 2024
@minjeong9919 minjeong9919 linked an issue Jul 15, 2024 that may be closed by this pull request
Copy link
Contributor

@bokeeeey bokeeeey left a comment

Choose a reason for hiding this comment

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

LGTM !

setIsOpenProfileCard(true);
};

useEffect(() => {}, [commentPositionTop]);
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 꼭 필요한건가요?

@@ -84,14 +100,19 @@ export default forwardRef<HTMLDivElement, CommentProps>(function Comment({ cardI

const handleClickReport = () => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

아무 동작도 안하는 함수인듯 한데 필요한가요?

Comment on lines 91 to 93
const { data, refetch } = useQuery<PostCardListResponseData>({
queryKey: ['postData', cardId],
queryFn: () => getPostDetail(cardId),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { data, refetch } = useQuery<PostCardListResponseData>({
queryKey: ['postData', cardId],
queryFn: () => getPostDetail(cardId),
const { data: 여기 네이밍해서 사용하는게 좋을거같기도 해요, refetch } = useQuery<PostCardListResponseData>({
queryKey: ['postData', cardId],
queryFn: () => getPostDetail(cardId),

Comment on lines +142 to +144
if (!userData) {
return toast.error('로그인이 필요합니다.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

로그인이 필요하면 바로 모달로 연결해주는건 어떤가요?

}
}, [isOpenProfileCard, userId, refetch, positionTop, isAboveProfile]);

return positionTop && positionTop === 0 ? null : (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return positionTop && positionTop === 0 ? null : (
return positionTop && positionTop === 0 ? null : (

여기서 positionTop이 없으면 그냥 null처리 하셔서 아무것도 안보여주시는식으로 설계하셨는데
아무것도 없는 데이터를 보여줘도 괜찮은 컴포넌트인가요?

Copy link
Contributor

Choose a reason for hiding this comment

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

그게 아니라면 사용처에서 useState로 관리하는게 더 낫지않을까요?

Comment on lines +58 to +60
<div>
<SpinLoading />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

div 태그가 아무런 기능을 하고있는것 같지 않아서 없어도 될거같아요

Copy link
Contributor

@armd482 armd482 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 +87 to +88
const result = await res.json();
const { data } = result;
Copy link
Contributor

Choose a reason for hiding this comment

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

구조 분해로 줄일 수 있을 것 같아요

Suggested change
const result = await res.json();
const { data } = result;
const { data }= await res.json();

Comment on lines +35 to +44
useEffect(() => {
const VIEWPORT_HEIGHT = window.innerHeight;

if (isOpenProfileCard) {
refetch();
if (positionTop && positionTop > VIEWPORT_HEIGHT / 2) {
setIsAboveProfile(true);
}
}
}, [isOpenProfileCard, userId, refetch, positionTop, isAboveProfile]);
Copy link
Contributor

Choose a reason for hiding this comment

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

구조적으로 봤을 때 의존성 변수가 많은 useEffect 내부에 refetch가 들어가 있습니다.
이는 불필요한 refetch가 발생할 수 있을 것 같아요.
제가 코드를 제대로 이해한 거라면, 저라면 userData를 사용처에서 prop으로 전달하고, 사용자가 마우스를 올렸을 때 데이터를 가져오는 식으로 할 것 같습니다. 아니면 refetch만 따로 다른 useEffect에 분리할 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

isAboveProfile을 의존성배열에 넣은 이유가 있을까요?
setState로 값이 변경되면서 한번 더 실행될 것 같은데...

Comment on lines +63 to +79
<div className={cn('profile-image')}>
<Image src={userInfo?.imgUrl || keydeukProfileImg} alt='프로필 이미지' fill sizes='12rem' />
</div>
<div className={cn('info-wrapper')}>
<p className={cn('nickname')}>{userInfo?.nickname || '사용자를 찾을 수 없습니다.'}</p>
<p>
<strong>email:</strong> {userInfo?.email}
</p>
<p>
<strong>birthday:</strong> {userInfo?.birth}
</p>
<p>
<strong>phone:</strong> {userInfo?.phone}
</p>
<p>
<strong>gender:</strong> {userInfo?.gender}
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

userInfo를 못 가져왔을 때는 아예 안 띄우는 식으로 또는 아예 사용자 정보를 찾을 수 없을 때 띄울 컴포넌트를 보여주는 것도 좋을 것 같습니다

width: 45rem;
height: 15rem;
padding: 1rem;
animation: fade-in 1s cubic-bezier(0.39, 0.575, 0.565, 1) both;
Copy link
Contributor

Choose a reason for hiding this comment

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

애니메이션 속도 너무 느린 것 같아요.
0.7초에서 0.5초 정도 어떠신가요?

Comment on lines 151 to 154
const removeEvent = addEnterKeyEvent({ element: { current: commentRef }, callback: handleSubmitComment });
return () => {
removeEvent();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

그냥 form으로 만드는 건 별로일까요? 그럼 따로 이벤트 등록 안 해도 될 것 같아요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Component 기능 개발
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[Feat] 다른 유저 프로필 보기
3 participants