-
Notifications
You must be signed in to change notification settings - Fork 35
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
[염성진] Sprint10 #291
The head ref may contain hidden characters: "Next-\uC5FC\uC131\uC9C4-sprint10"
[염성진] Sprint10 #291
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.
수고많으셨습니다! :-) 함수 역할마다 분리하려고 고민 하신게 보입니다! 잘하고계신거에요!
<Image fill src={image} className={S.image} alt="아이템 이미지" /> | ||
<Image | ||
fill | ||
src={image || `/images/icon/ic_null_user_profile_image.png`} |
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.
좋습니다. 이미지가 없을떄에 보여줄 default image 설정 잘 해주셨어요!
function formatDate(dateString: string): string { | ||
const date = new Date(dateString); | ||
const options: Intl.DateTimeFormatOptions = { | ||
year: "numeric", | ||
month: "2-digit", | ||
day: "2-digit", | ||
}; | ||
const formattedDate = date | ||
.toLocaleDateString("ko-KR", options) | ||
.replace(/\s+/g, "") | ||
.replace(/\//g, "."); | ||
|
||
return formattedDate.endsWith(".") ? formattedDate.slice(0, -1) : formattedDate; | ||
} |
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.
시간을 나타내는 함수를 잘 만들어주셨네요
한땀한땀 만드는것도 꼭 필요한 경험이에요!
dayjs 같은 라이브러리를 쓰면
dayjs(dateString).format('YYYY.MM.DD')
한줄로 같은 효과를 낼 수 있어요. 적시적소에 라이브러리를 활용하는것도 개발 시간을 줄여주는 방법이 될 수 있어요 :-)
const nextProduct = res.data; | ||
setProduct(nextProduct); | ||
} catch (error) { | ||
console.error(`유효하지 않은 주소입니다.`); |
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.
유저는 콘솔을 몰라서 유효하지 않은 주소라는건 볼 수가 없을것같아요.
아마 잘못된 주소를 입력하면 board로 이동해버리는게 어찌보면 잘못된것이라고 생각할 수 도 있을것같아요.
콘솔에만 띄우는것 보단 유저한테 잘못된 주소를 입력했다고 알려주는건 어떨까요? :-)
|
||
useEffect(() => { | ||
getProduct(id); | ||
setLoading(false); |
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.
loading 상태관리는 product를 가져오는 getProduct 안에서 setLoading이 있어야 할것같아요.
useEffect안에서는 getProduct 함수가 종료되면 setLoading이 불리게 되는데 그러면 getProduct의 resolve,reject 여부랑 상관없이 loading상태가 false로 설정됩니다.
데이터가 아직 로드되지 않았거나, 에러가 있어도 로딩 상태가 종료된 것처럼 보일 수 있어요.
loading은 데이터를 받아오기 전에 로딩중 인지 알려주고, 데이터를 다 받으면 로딩 화면이 안보이면 되니까 try,catch,finally에서 어디에서 호출해야 할지 판단 해보시고 수정 해 보세요 :-)
async function getProduct(id: number) { | ||
const res = await axios.get(`/articles/${id}/comments?limit=50`); | ||
const nextComments = res.data.list; | ||
console.log(nextComments); |
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.
local 에서 테스트가 끝난 console.log는 commit 전에 삭제 해주시는게 좋습니다 :-)
function DetailBoardComments() { | ||
const [comments, setComments] = useState<ItemsListType[]>([]); | ||
const [loading, setLoading] = useState(true); | ||
const [values, setValues] = useState(""); |
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.
value는 개발 하는 모든 곳에서 쓰일 수 있는 범용적인 이름이라, 구체적인 이름으로 적어주시는게 좋을것같습니다.
data, item, value 모두 같아요. (어떤)value 인지만 적어도 구분하기 쉬울 것 같아요!
// 테스트를 위해 추가한 동작 | ||
const handleSubmit = (e: FormEvent<HTMLButtonElement>) => { | ||
e.preventDefault(); | ||
console.log(values); | ||
}; |
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.
이런 코드에는
//TODO :
를 추가해서 언제 다시 만들껀지, 부연설명을 적어주시는게 팀 단위 개발할때는 필요합니다. 만약, 이 커밋이 main에 merge 되고나서 다른 팀원들이 이 코드를 보는 시점에는 배경지식이 없을수도 있기 때문이에요.
useEffect(() => { | ||
function validation() { | ||
const valueCheck = values.length > 0; | ||
return valueCheck; | ||
} | ||
const isValid = validation(); | ||
setPass(isValid); | ||
}, [values]); |
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.
value로 계산할 수 있는 상태는 추가적인 상태를 만들 필요가 없어보입니다.
className={
${S.commentSubmitButton} ${value.length>0 ? S.pass : ""}}
value로 계산한 값을 또 다른 state에 담아서 상태관리 하면 두 상태에 결합이 강해져서 나중에 분리해야할 상황이 있다면 분리하기 어려울것같아요.
react 공식문서에 어떤걸 state로 만들어야 하는지, 하면 안되는지 설명서가 있습니다 여길 참고 해 보세요
// 등록 버튼 클릭 시 제출 | ||
const handleSubmit = (e: FormEvent<HTMLFormElement>) => { | ||
e.preventDefault(); | ||
console.log(values); |
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.
요기 console.log도 삭제가 필요하겠네요
return; | ||
} | ||
const image = values.images; | ||
console.log(image); |
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.
요기 console.log도 삭제가 필요하겠네요
요구사항
기본
상품 등록 페이지
상품 상세 페이지
주요 변경사항
스크린샷
멘토에게