[Feat] content-detail-page #36
Conversation
YouD0313
requested review from
HANTAEDONG,
HongbiKo,
dlcks0601,
hwookim and
peppertown
YouD0313
added
✨ Feature
hwookim
left a comment
hwookim
left a comment
There was a problem hiding this comment.
고생하셨습니다!!! 👍🏼 👍🏼
react query까지 아주 멋지게 사용해주셨네요.
그런데, pnpm-lock.yaml에 변경 사항이 있는 건 왜일까요?
node 버전이 유지님만 다른걸까요?
한 번 팀원들과 이야기해보고 node 버전을 맞춰보는 것도 중요한 작업이랍니다. 😄
| const response = await httpClient.get<IContent>(`/api/posts/${content_id}`); | ||
| return response.data; | ||
| } catch (e) { | ||
| console.log('에러났어', e); |
| export interface FetchContentParams { | ||
| content_id: string | undefined; | ||
| } |
There was a problem hiding this comment.
| export interface FetchContentParams { | |
| content_id: string | undefined; | |
| } | |
| export interface FetchContentParams { | |
| content_id?: string; | |
| } |
undefined가 올 수 있는 경우에는 ?를 이용하여 표시하기도 한답니다.
근데, 단 하나의 인자만 온다면 굳이 interface로 선언할 필요가 있을까요? 🤔
type으로 선언해도 충분할 것 같기도 합니다.
그렇다면 ?는 못쓰겠지만요 😅
| export default function QuestionContent({ content }: Props) { | ||
| return <QuestionContentStyle>{content}</QuestionContentStyle>; | ||
| } | ||
|
|
||
| const QuestionContentStyle = styled.div` | ||
| font-size: 15px; | ||
| `; |
There was a problem hiding this comment.
이정도 컴포넌트라면 단순 스타일 컴포넌트만으로 부모에서 만들어 써도 되지 않았을까 싶기는 하네요 🤔
나중의 확장성을 위해 미리 분리해두신 걸까요? 👀
| ); | ||
| } | ||
|
|
||
| const QuestionCommentStyle = styled.div` |
There was a problem hiding this comment.
스타일 컴포넌트와 classname을 혼합해서 쓰시는 이유가 있을까요? 👀
CommentContainer
CommentTitle
CommentInput
같은 형식으로 styled-component만 써도 충분히 표현할 수 있을 것 같아요 👀
물론 class name을 혼합하여 쓰는 것이 잘못된 방식은 아닙니다.
다만 여러 방식을 혼재해서 쓸 경우 나중에는 헷갈리게 되거나 유지보수가 힘들어질 수도 있어서요.
특별한 경우가 아니라면 하나의 도구를 사용하는 편이 좋다고 생각해요 😄
| const { data } = useQuery({ | ||
| queryKey: ['content', content_id], | ||
| queryFn: async () => await fetchContent({ content_id }), | ||
| enabled: !!content_id, |
There was a problem hiding this comment.
content_id가 혹시혹시 0이면 어떡하죠...?! 👀
물론 사실 그럴 일 없다는 백엔드의 말 하나면 해결되긴 합니다 ㅋㅋ 하지만 혹시...?
| @@ -0,0 +1,27 @@ | |||
| export interface IPost { | |||
There was a problem hiding this comment.
IPost라는 네이밍은 interface임을 나타낼 수 있는 괜찮은 네이밍이지만,
전반적인 프로젝트를 봤을 때 다른 인터페이스들과는 달라보이네요.
한 번 통일시킬 필요가 있어보여요.
만일 새로운 사람이 IPost를 먼저 보고 다른 인터페이스를 본다면, 우선적으로 인터페이스가 아니라고 여길 수 있으니깐요 👀
가장 중요한 것은 팀 컨벤션!
#️⃣연관된 이슈
📝작업 내용
스크린샷 (선택)
💬리뷰 요구사항(선택)