-
Notifications
You must be signed in to change notification settings - Fork 24
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
[함헌규] sprint8 #112
The head ref may contain hidden characters: "next-\uD568\uD5CC\uADDC-sprint8"
[함헌규] sprint8 #112
Conversation
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.
상품 리스트의 경우 화면크기에 따라 take의 값이 달라져야 하는데 화면크기를 확인하는 훅을 사용하기 위해 클라이언트 컴포넌트로 처리했습니다. 이 부분도 서버사이드 렌더링을 적용할 수 있는 방법이 있을까요?
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.
말씀하신 것처럼 서버에서는 클라이언트의 화면 크기에 따른 요청 갯수를 미리 파악할 수 없기 때문에 서버 사이드만으로 처리하기에는 한계가 있습니다. 페이지가 최초로 로드될 때만 서버에서 처리하고 이후로는 클라이언트에서 처리하는 방식으로 구현하면 SEO나 CSR로 구현할 때 페이지 로드 후 데이터를 불러오기까지 빈 데이터가 노출되는 지연을 줄일 수 있을 것 같네요.
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.
이번 과제를 정말 열심히 작업하셨다는 사실이 그대로 느껴지는 것 같네요.
심화 요구사항에 지금까지 작성한 코드를 Next로 마이그레이션 했다에 체크를 하지 않으셨는데, 이 정도면 전부 Next로 마이그레이션하신 것 아닌가요? ㅋㅋ
컴포넌트를 더 나누라고 말하기 힘들 정도로 이미 잘게 나누어 놓으셔서 컴포넌트와 커밋 메시지, 단위에 관해서도 더 드릴 코멘트는 없을 것 같네요.
컴포넌트마다 상수를 적극적으로 활용하신 부분도 좋습니다.
고생하셨습니다.
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.
말씀하신 것처럼 서버에서는 클라이언트의 화면 크기에 따른 요청 갯수를 미리 파악할 수 없기 때문에 서버 사이드만으로 처리하기에는 한계가 있습니다. 페이지가 최초로 로드될 때만 서버에서 처리하고 이후로는 클라이언트에서 처리하는 방식으로 구현하면 SEO나 CSR로 구현할 때 페이지 로드 후 데이터를 불러오기까지 빈 데이터가 노출되는 지연을 줄일 수 있을 것 같네요.
src/services/api/article.ts
Outdated
const queryParams = new URLSearchParams({ | ||
skip: skip.toString(), | ||
take: take.toString(), | ||
orderBy, | ||
...(word && { word }), | ||
}); | ||
try { | ||
const { data } = await axiosInstance.get<GetArticleListResponse>( | ||
`/articles?${queryParams.toString()}`, | ||
); |
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.
axios를 사용하면 query string은 params로 처리할 수 있는데, URLSearchParams로 처리하신 이유가 무엇인가요?
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 handleAddTag = (e: React.KeyboardEvent<HTMLInputElement>) => { | ||
if (e.key === 'Enter') { | ||
e.preventDefault(); | ||
setTagError(''); | ||
if (e.nativeEvent.isComposing) return; | ||
const newTag = tagInput.trim(); | ||
|
||
if (newTag.length > 5 || newTag.length < 1) { | ||
return setTagError('5글자 이내로 입력해주세요'); | ||
} | ||
|
||
if (newTag !== '' && !tags.includes(newTag)) { | ||
setValue('tags', [...tags, newTag]); | ||
return setTagInput(''); | ||
} | ||
|
||
setTagError('이미 등록된 태그입니다.'); | ||
} | ||
}; | ||
|
||
const handleRemoveTag = (indexToRemove: number) => { | ||
setValue( | ||
'tags', | ||
tags.filter((_, index) => indexToRemove !== index), | ||
); | ||
}; |
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.
이런 핸들러 함수와 state를 묶어서 커스텀 훅으로 정의해보는 건 어떨까요?
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.
말씀해주신대로 훅을 만들어서 사용하도록 수정했습니다 감사합니다!
{isEditMode ? '게시글 수정하기' : '게시글 쓰기'} | ||
</h2> | ||
<CommonBtn | ||
disabled={!formComplete} | ||
type='submit' | ||
> | ||
{isEditMode ? '수정' : '등록'} |
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.
WriteForm 컴포넌트 내부에서 출력한 텍스트를 판단하기 위해서 pathname까지 가져오고 있는데, 이 부분을 page 레이어에서 판단 후 타이틀과 버튼 내용을 props로 전달하는 방식으로 처리하면 WriteForm 내부에서 직접적인 라우터 의존도 줄일 수 있지 않을까요?
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.
writeForm에 variant prop을 추가하고 각 페이지에서 수정인지 등록인지 입력하는 방식으로 수정했습니다! 감사합니다!
const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => { | ||
e.preventDefault(); | ||
const formData = new FormData(e.currentTarget); | ||
const comment = formData.get('content') as string; | ||
createCommentMutation.mutate(comment); | ||
}; |
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.
다른 쪽에서는 form을 처리할 때 hook form 라이브러리를 사용하셨던 것 같은데, 여기서는 FormEvent로 처리하셨군요.
comment를 등록할 때는 HTMLEvent로 처리한 이유가 구현이 간단해서인가요?
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.
다른 사람(협업자나 컨트리뷰터)에게 공유할 수 있는 명확한 기준이 있다면 괜찮습니다. 어쩔 땐 A고, 어쩔 땐 B고, 기준이 없는 대안이 늘어나다 보면 코드가 통일되지 않고 제각각이 될 수 있거든요.
그게 제가 현재 사용하고 있는 Django 프레임워크의 가장 큰 문제....
- edit/write 를 통해 글 작성 폼인지 수정 폼인지 결정하고 이에 따라 표시되는 텍스트를 결정
- skip이 0일 경우 searchParams에 skip을 추가하지 않도록 설정
요구사항
기본 요구사항
공통
자유 게시판 페이지
게시글 등록 & 수정 페이지
게시글 상세 페이지
심화 요구사항
공통
스크린샷
배포 주소 : https://3-sprint-mission-fe.vercel.app/community
(서버가 켜지는 시간이 필요해 처음엔 오류가 뜰 수 있습니다.)
게시글 목록페이지
게시글 상세페이지
게시글 쓰기 페이지
멘토에게