-
Notifications
You must be signed in to change notification settings - Fork 3
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
[#107] Feat: 유저페이지 구현 #132
Conversation
🚀 Storybook is deployed! View it here: https://6621243cb767d61b79ad0f3c-cyebvxgage.chromatic.com/ |
text: string; | ||
isNickname?: boolean; | ||
}) => { | ||
const posts = useGetPostHistory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useGetPostHistory 구현 내용을 보면, url 의 마지막 path 를 가져와서 ('mycomment' |'mylike' 등) 해당 카테고리에 관련된 posts 를 fetch 해오는것 같습니다. 👍
근데, 어차피 myComment 나 mylike 등은 page.tsx 가 있어 page.tsx 접속시 해당 카테고리인것이 보장되므로 useHook 을 한번 더쓰고 path 를 가져오는대신 바로 page.tsx 에서 mycomment, mylike 로 params 를 주어 fetch 시키는것도 좋을것 같습니다.
이렇게 하면 PostingList 컴포넌트에서 한뎁스 더 들어가지 않아도될것같습니다.
또한, PostingList 에 posts 를 props 로 받고 각 my 관련 컴포넌트에서 fetch 시켜 넘겨주면, PostingList 는 단순히 출력만 하면 되므로 fetch 기능과 책임이 분리되는장점도 있을것같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
근데 [userId]/[params]/page.tsx하면 결국 같은뎁스 아닌가요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 제가 글을 잘 못쓴것 같네요!
myComment, myLike 페이지 컴포넌트에서 PostingList 컴포넌트에 각각 'myComment', 'myLike' 를 fetch 하는 훅에 params 로 넘겨주어 PostingList 컴포넌트의 props 로 데이터를 준다면 안에 useGetPostHistory 를 제거해도 될것같단 말이었습니다.
이렇게 한다면 useGetPostHistory 훅에서 path name 을 굳이 가져오지 않아도될것 같단 생각이 들었습니다. 그렇게된다면 useGetPostHistory 를 사용하면서 생기는 뎁스를 줄일수있다고 생각했습니다.
page.tsx 에서 단방향으로 data 를 내려주면 될것같은데, PostingList 컴포넌트 내부에서 추상화된 useGetPostHistory의 usePathname 을써서 다시 pathName 을 얻어오는것이 어색하다고 느껴졌습니다.
또한 useGetPostHistory 없앤다면, fetch data 를 가져오는 경우에대해서 의존하지 않아도되는 이점이 생깁니다. 결과로, PostingList 컴포넌트는 단순히 출력만 하는 컴포넌트가 되어 나중에 useGetPostHistory 의 변경을 신경쓰지 않아도된다는 장점이생기고요.
</div> | ||
</div> | ||
</div> | ||
{!userId ? <MyPageButton /> : <UserPageButton />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각 버튼 컴포넌트를 분리해주셨군요!👍
저는 개인적으로 myPageButton 과 userPageButton 은 ProfileBlock 내 보다, 각 페이지에서 담당해주는게 더 좋을것 같단생각이듭니다.
만약 새로운 버튼을 추가한다고 생각한다면, 예를들어 adminPageButton 이면, userId 로 분기뿐만 아니라 author 인지도 체크해서 여러 분기가 생길것같습니다.
이때 profileBlock 을 사용하는쪽인 userPage, myPage 쪽에서 각각 기능하는 버튼을 내려주어,처래해주어도 좋을것같습니다.
이렇게하면 page 에 의존하는 버튼들을 profileBlock 이라는 컴포넌트에서 책임지는것이 아니라, 각 page 에서 button 들을 책임하게되어 응집도를 높일수있을것 같습니다. 또한 profileBlock 컴포넌트는 분기문을 만들지 않아도되니, 변화에 보호될것같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당내용에 대해 고민을 해보았는데요, 저는 개인적으로
버튼은 프로필 블럭안에 포함되어있어야 한다고 생각합니다
profile/page
를 보면 profileblock과 loginInfo가 분리되어있는데, 이 관점에서 생각해보면 loginInfo와 (profileblock, button)이 분기처리 되어야 하는데, 버튼은 프로필 블럭안에 포함되어있는 요소여야하지 않을까? 라는 의견이 드네요.
물론 페이지별로 버튼이 다르게 렌더링되는것이니 고유의 책임에 따라 페이지를 구분하는것도 좋겠지만, 하나로 묶어서 관리하는것도 의존성에 크게 떨어지지 않는다는 의견입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profile/page 를 보면 profileblock과 loginInfo가 분리되어있는데, 이 관점에서 생각해보면 loginInfo와 (profileblock, button)이 분기처리 되어야 하는데, 버튼은 프로필 블럭안에 포함되어있는 요소여야하지 않을까? 라는 의견이 드네요.
이부분에 대해 궁금한 점이있습니다. ProfileBlock 컴포넌트에 children 혹은 props 로 button 을 내려준다면, ProfileBlock 내부의 분기문을 제거해도된다고 말씀을 드린것인데, 혹시 이렇게 이해해주신게 맞을까요? 맞다면, loginInfo 와 ProfileBlock 간의 분기는 버튼들과는 상관없어보이는것같은데, 어떤부분에서 버튼 로직들이 프로필 블럭안에 포함되어있어야 한다고 생각이드셨을까요??!
하나로 묶어서 관리하는것도 의존성에 크게 떨어지지 않는다는 의견입니다.
나중에 크게 버튼들의 종류가 많아질것같단 생각이 들지않아 말씀주신 의견에 동의합니다!
a49aaab
to
b961728
Compare
🚀 Storybook is deployed! View it here: https://6621243cb767d61b79ad0f3c-dxgihtagvv.chromatic.com/ |
🚀 Storybook is deployed! View it here: https://6621243cb767d61b79ad0f3c-itfcfudlim.chromatic.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다:)
@@ -23,7 +52,7 @@ interface ProfileResponse { | |||
// }; | |||
|
|||
const getProfile = (userId: string) => { | |||
return api.get<ProfileResponse>({ | |||
return api.get<Body>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Body보다 더 구체적인 변수명이면 좋을 거 같은데 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네네 그렇게 할게요 ! 병합하는과정에서 꼬인게 있나봅니다..
const postingCategory: PostingIconProps[] = [ | ||
{ icon: 'write', name: '작성 글', routingPath: 'myPosting' }, | ||
{ icon: 'check-circle', name: '투표 내역', routingPath: 'myVoteHistory' }, | ||
{ icon: 'like', name: '좋아한 투표', routingPath: 'myLike' }, | ||
{ icon: 'message-2', name: '내 댓글', routingPath: 'myComment' }, | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 데이터는 상수니까 밖에서 정의하면 좋을 거 같습니다:)
💬 Issue Number
🤷♂️ Description
📷 Screenshots
👻 Good Function
📋 Check List
📒 Remarks