Skip to content
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

Conversation

MELATONIN99
Copy link
Collaborator

요구사항

기본

상품 등록 페이지

  • 게시글 등록 페이지 주소는 “/addboard” 입니다.
  • 게시판 이미지는 최대 한개 업로드가 가능합니다.
  • 각 input의 placeholder 값을 정확히 입력해주세요.
  • 이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
  • 회원가입, 로그인 api를 사용하여 받은accessToken을 사용하여 게시물 등록을 합니다.
  • ‘등록’ 버튼을 누르면 게시물 상세 페이지로 이동합니다.
    상품 상세 페이지
  • 게시글 상세 페이지 주소는 "/board/{id}"입니다.
  • 댓글 input 값을 입력하면 '등록'버튼이 활성화 됩니다.
  • 자유게시판 페이지에서 게시글을 누르면 게시물 상세 페이지로 이동합니다.

주요 변경사항

  • 반응형으로 구현하였습니다.
  • 추후 커밋으로 좀 더 보완하겠습니다!

스크린샷

image
image
image

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@MELATONIN99 MELATONIN99 requested a review from 201steve August 16, 2024 09:41
@MELATONIN99 MELATONIN99 added 미완성🫠 죄송합니다.. 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. labels Aug 16, 2024
@MELATONIN99 MELATONIN99 self-assigned this Aug 16, 2024
Copy link
Collaborator

@201steve 201steve left a 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`}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다. 이미지가 없을떄에 보여줄 default image 설정 잘 해주셨어요!

Comment on lines +26 to +39
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;
}
Copy link
Collaborator

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(`유효하지 않은 주소입니다.`);
Copy link
Collaborator

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);
Copy link
Collaborator

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);
Copy link
Collaborator

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("");
Copy link
Collaborator

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 인지만 적어도 구분하기 쉬울 것 같아요!

Comment on lines +53 to +57
// 테스트를 위해 추가한 동작
const handleSubmit = (e: FormEvent<HTMLButtonElement>) => {
e.preventDefault();
console.log(values);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 코드에는

//TODO :
를 추가해서 언제 다시 만들껀지, 부연설명을 적어주시는게 팀 단위 개발할때는 필요합니다. 만약, 이 커밋이 main에 merge 되고나서 다른 팀원들이 이 코드를 보는 시점에는 배경지식이 없을수도 있기 때문이에요.

Comment on lines +82 to +89
useEffect(() => {
function validation() {
const valueCheck = values.length > 0;
return valueCheck;
}
const isValid = validation();
setPass(isValid);
}, [values]);
Copy link
Collaborator

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);
Copy link
Collaborator

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요기 console.log도 삭제가 필요하겠네요

@201steve 201steve merged commit 9f9362f into codeit-bootcamp-frontend:Next-염성진 Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants