-
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
[#125] Feat: 댓글 수정 삭제 기능 구현 #140
base: main
Are you sure you want to change the base?
Conversation
🚀 Storybook is deployed! View it here: https://6621243cb767d61b79ad0f3c-nxdocafiqs.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.
수혁님 수고하셨습니다:)
<DropdownMenu.Root> | ||
<DropdownMenu.Trigger asChild> | ||
<button> | ||
<Icon id='more-square' /> |
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.
aria-label추가하면 좋을 거 같습니다.
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.
감사합니다! 반영하겠습니다.
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.
CommentReplyItemMenu, CommentRootItemMenu 컴포넌트가 중복되는 부분이 좀 있는 거 같은데 둘이 분리하는 방법을 선택하신 이유가 있으실까요?
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.
한파일 에서 분리하면 조건 문이 생길 것 같습니다. 추후에 다른 commentItemMenu 의 종류가 생긴다면 더 복잡해질 것같습니다. 변경의 크기도 reply, root 정도의 크기로 정하는게 좋을것 같아 파일단위를 정했습니다.
중복되는 부분을 함수로 빼내는 식으로 해볼까 생각이 드는데 혹시 어떻게 생각하시나요?!
|
||
const CommentModalButton = () => { | ||
const router = useRouter(); | ||
const handleClick = () => { |
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.
함수명을 더 구체화하는 것도 좋을 거 같습니다:)
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.
넵넵 수정해보겠습니다!
isSelected && 'bg-gray-accent2', | ||
)} | ||
> | ||
<Icon id='reply' className=' text-gray-accent3' /> |
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.
aria-label추가해주면 좋을 거 같습니다.
<DropdownMenu.Portal> | ||
<DropdownMenu.Content | ||
onCloseAutoFocus={e => e.preventDefault()} | ||
className=' font-text-3-rg flex flex-col gap-2 rounded-lg bg-[#2d3033] p-2 text-white' |
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.
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.
오 그러네요 알겠습니다!
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.
제가 당장 코드를 돌려보면서 리뷰를 남긴게 아니라 부족한 부분이 있을 수 있을 것 같습니다 ㅠㅡㅠ 그래도 코드를 가독성있게 짜는건 너무 많이 좋아진 것 같네요 !
저녁에 한번 회의하면서 수정 해보면 좋을 것 같슴다 고생하셨어요 !!
return ( | ||
<DropdownMenu.Root> | ||
<DropdownMenu.Trigger asChild> | ||
<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.
혹시 여기 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.
오 아니요 radix 문서 코드를 가져왔습니다.
asChild 를 제거하고 Icon 만 적용해보겠습니다!
const CommentItemMenuButton = ({ | ||
label, | ||
onClick, | ||
}: CommentItemMenuButtonProps) => { | ||
return <DropdownMenu.Item onSelect={onClick}>{label}</DropdownMenu.Item>; | ||
}; |
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 CommentItemMenuButton = ({ | |
label, | |
onClick, | |
}: CommentItemMenuButtonProps) => { | |
return <DropdownMenu.Item onSelect={onClick}>{label}</DropdownMenu.Item>; | |
}; | |
const CommentItem = ({ | |
label, | |
onSelect, | |
}: CommentItemMenuButtonProps) => { | |
return <DropdownMenu.Item onSelect={onSelect}>{label}</DropdownMenu.Item>; | |
}; |
기존에 Item을 그대로 사용해도 좋을 것 같은데 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.
아니요 없습니다.! 아마 DropdownMenu.Item 이것과 겹친다 생각해서 Button 으로 수정한것 같습니다!
수정해보겠습니다!
)} | ||
> | ||
<Icon id='reply' className=' text-gray-accent3' /> | ||
<CommentItem |
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.
이부분은 고민이 되는데, tanstack query
의 캐싱기능을 잘 활용해보면 좋을 것 같은데
해당 props들을 드릴링으로 내리는 것 보단 CommentItem에서 훅을 호출해서 데이터를 꺼내보는건 어떨까요 ??
그래야 Suspense 씌울 때 로딩처리가 더 깔끔하지 않을까? 싶습니다 !
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.
오 그러네요! 감사합니다 수정해보겠습니다!
createdAt={createdAt} | ||
isOwnComment={isOwnComment} | ||
> | ||
<CommentReplyItemMenu replyId={replyId} isOwnComment={isOwnComment} /> |
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.
해당내용도 위랑 동일합니다 : )
comment, | ||
}: CommentRootItemProps) => { | ||
const { commentId, nickname, content, createdAt, userId } = comment; | ||
const commentIdFromSearchParams = useSearchParams().get('commentId'); |
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.
이부분은 단순 궁금증? 이었는데, commentId는 parameter가 아니라 쿼리스트링으로 관리한 이유가 있을까요 ?
기존에 ID값들은 모두 parameter로 한 것 같아서요 !
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.
제가 실제 환경에서 띄워놓고 테스팅 하면 좋을 것 같은데 현재 그걸 못해봐서 .. 따로 여쭤봅니다 !
혹시 RootItem이랑, ReplyItem이랑 하나의 파일에서 분리하는건 불가능했을까요 ?
뭔가 동작은 같고, UI랑 드롭다운만 다를 것 같아 여쭤봅니다
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.
가능할것같습니다!
다만 한파일 에서 분리하면 조건 문이 생길 것 같습니다. 추후에 다른 commentItem 종류가 생긴다면 더 복잡해질 것같습니다. 변경의 크기도 reply, root 정도의 크기로 정하는게 좋을것 같아 파일단위를 정했습니다.
그래서 중복을 제거하는 방법으로, 파일을 통합하여 조건문으로 분리 하지말고 중복되는 부분만 함수로 빼는 것이 어떨까 싶은데 ,, 혹시 어떻게 생각하실까요?!
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.
commentId 는 CommentForm 에서도 사용됩니다.
commentId 가 세그먼트 로 관리되면, CommentForm 이 특정 페이지에서 관리 되어 모달을 사용하더라도 CommentList 나 post 정보 ui 가 사라집니다.
구현하고나서 Parallel Routes 라는걸 알았는데, 요걸로 리팩토링 하면 세그먼트로 작성해도될것같습니다.
<div className='font-h3-sm border-t border-gray-accent2 px-4 py-5'> | ||
댓글 {commentSize} | ||
</div> | ||
<div className=' flex flex-col divide-y divide-gray-accent2'> |
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.
<div className=' flex flex-col divide-y divide-gray-accent2'> | |
<div className='flex flex-col divide-y divide-gray-accent2'> |
공백있습니다 !
댓글 {commentSize} | ||
</div> | ||
<div className=' flex flex-col divide-y divide-gray-accent2'> | ||
{comments.map((rootComment, idx) => ( |
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.
rootComment 구조분해할당 해서 쓰는건 어떨까요 ? 가독성이 많이 좋아질 것 같슴다
export type CommentFormValues = { | ||
comment: string; | ||
}; | ||
|
||
const CommentForm = ({ | ||
isOpenModal, | ||
onCloseModal: closeModal, | ||
}: CommentFormProps) => { | ||
const { postId } = useParams() as { postId: string }; | ||
const method = useSearchParams().get('method') as 'create' | 'update' | null; | ||
const target = useSearchParams().get('target') as 'root' | 'reply' | null; | ||
const comments = useGetCommentsAPI(postId); | ||
const commentId = useSearchParams().get('commentId'); |
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.
export type CommentFormValues = { | |
comment: string; | |
}; | |
const CommentForm = ({ | |
isOpenModal, | |
onCloseModal: closeModal, | |
}: CommentFormProps) => { | |
const { postId } = useParams() as { postId: string }; | |
const method = useSearchParams().get('method') as 'create' | 'update' | null; | |
const target = useSearchParams().get('target') as 'root' | 'reply' | null; | |
const comments = useGetCommentsAPI(postId); | |
const commentId = useSearchParams().get('commentId'); | |
type MethodQueryStringType = 'create' | 'update' | null | |
const CommentForm = ({ | |
isOpenModal, | |
onCloseModal: closeModal, | |
}: CommentFormProps) => { | |
const { postId } = useParams() as { postId: string }; | |
const method = useSearchParams().get('method') as MethodQueryStringType | |
const target = useSearchParams().get('target') as 'root' | 'reply' | null; | |
const comments = useGetCommentsAPI(postId); | |
const commentId = useSearchParams().get('commentId'); |
간단한 comment는 그냥 아래서 단언으로 선언해주고 유니언타입을 오히려 선언해주는게 가독성이 더 좋아질 것 같아요
}, [isUpdateComment, selectedComment, setValue, setFocus]); | ||
|
||
return ( | ||
<div className='p-4'> |
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.
div에서 패딩주고 form에서 또주면 이상한 것 같아요 ! form에서 마진을 주던지와 같이 해서 수정해볼 수 있지않을까요 ?
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
-.Chrome.2024-06-28.21-27-47.mp4
두번째 결과물(대댓글,댓글 하이라이트)
👻 Good Function
📋 Check List
📒 Remarks