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 #268

Conversation

mxkxx1011
Copy link
Collaborator

@mxkxx1011 mxkxx1011 commented Jul 19, 2024

요구사항

기본

  • 상품 등록 페이지 주소는 “/addboard” 입니다.
  • 게시판 이미지는 최대 한개 업로드가 가능합니다.
  • 각 input의 placeholder 값을 정확히 입력해주세요.
  • 이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.

주요 변경사항

스크린샷

멘토에게

  • 하다가 궁금한 부분에 대해서 주석 달아놨습니다!
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@mxkxx1011 mxkxx1011 changed the base branch from main to Next-최민경 July 19, 2024 15:34
@mxkxx1011 mxkxx1011 requested a review from lhc0506 July 19, 2024 15:35
@mxkxx1011 mxkxx1011 added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Jul 19, 2024
@mxkxx1011 mxkxx1011 removed the 미완성🫠 죄송합니다.. label Jul 21, 2024
};

function CommentForm() {
const [isFilled, setIsFilled] = useState<boolean>(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const [isFilled, setIsFilled] = useState<boolean>(false);
const [isFilled, setIsFilled] = useState(false);

state의 타입이 원시타입이고 기본값이 들어간다면 타입은 생략이 가능합니다!
단, 이부분은 사람마다 취향이있는것같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

disabled처리를 위해서 isFilled를 구현해주셨어요!👍
그런데 꼭 state와 effect로 처리를 해야할까요?!

commentValue자체가 state라서 상태가 변하니

state나 effect 없이
const isFilled = commentValue.content.trim() !== '';
를해줘도 commentValue가 바뀔때마다 isFilled값은 바뀔 것 같아요!

Comment on lines +20 to +23
setCommentValue((prevValue) => ({
...prevValue,
[name]: value,
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

setState((prev) =>)를 사용하면 이전 값을 가지고서 잘 사용이 가능해요!
그래서 작성해주신 코드를 사용하면 객체를 override하게 되어 유용하게 사용된답니다.
그런데 지금은 속성도 하나 입력필드도 하나여서
setCommentValue({ content: e.target.value });
만으로도 가능할 것 같아요!

Comment on lines +14 to +20
<Image
src='/images/ic_kebab.svg'
alt='kebab'
height={24}
width={24}
onClick={handleKebabToggle}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Image
src='/images/ic_kebab.svg'
alt='kebab'
height={24}
width={24}
onClick={handleKebabToggle}
/>
<button onClick={handleKebabToggle}>
<Image
src='/images/ic_kebab.svg'
alt='메뉴 열기'
height={24}
width={24}
/>
</button>

이렇게 버튼요소 감싸 주는것이 접근성 측면에서 조금 좋습니다!

import { useState } from 'react';
import { Dropdown } from './Dropdown';

function Kebab() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

케밥 컴포넌트를 따로 만들어서 안에서 드롭다운까지 잘 만들어주셨어요!

그런데 한가지 아쉬운점은 지금 현재 이컴포넌트는 '수정', '삭제' 만 가능한 케밥이에요. 만약 다른 선택지들이 와야한다면 복붙을 해서 따로 만들어줘야하죠!
이부분을 공통화를 한번 해보시는 것 을 추천드립니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

또 여유가 되신다면 외부클릭시 닫히는 로직도 추가해보세요!

import Kebab from '../Kebab';

function Board({ article }: TArticle) {
const { id, title, content, image, writer, likeCount, createdAt, updatedAt } =
Copy link
Collaborator

Choose a reason for hiding this comment

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

사용되지 않는 변수들이 있네요!

Comment on lines +25 to +34
// 여기에서 <HTMLInputElement> 와 <HTMLTextAreaElement> 이 부분만 다른데 <T> 이런식으로 할 방법은 없을까요..?!!
const handleInputChange = (e: ChangeEvent<HTMLInputElement>) => {
const { name, value } = e.target;
handleChange(name, value);
};

const handleInputTextAreaChange = (e: ChangeEvent<HTMLTextAreaElement>) => {
const { name, value } = e.target;
handleChange(name, value);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

한다면

Suggested change
// 여기에서 <HTMLInputElement> 와 <HTMLTextAreaElement> 이 부분만 다른데 <T> 이런식으로 할 방법은 없을까요..?!!
const handleInputChange = (e: ChangeEvent<HTMLInputElement>) => {
const { name, value } = e.target;
handleChange(name, value);
};
const handleInputTextAreaChange = (e: ChangeEvent<HTMLTextAreaElement>) => {
const { name, value } = e.target;
handleChange(name, value);
};
const handleInputChange = <T extends HTMLInputElement | HTMLTextAreaElement>(
e: ChangeEvent<T>
) => {
const { name, value } = e.target;
handleChange(name, value);
};

이렇게도 가능할 것 같습니다!

혹은 event를 argument로 받지않고 name과 value를 받게하고 onChange에서 직접 익명함수로 호출할 수도 있겠구요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

생각해보니 굳이 제네릭을 사용하지않고 HTMLInputElement | HTMLTextAreaElement로만 끝내도 여기서는 무방하구요!


return (
<main className={styles.main}>
<form>
Copy link
Collaborator

Choose a reason for hiding this comment

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

form을 만든 만큼 submit을 사용해보시면 어떨까요?!

Comment on lines +14 to +23
export async function getStaticProps() {
const res = await axios.get(`/articles?page=1&pageSize=3&orderBy=like`);
const articles = res.data.list;

return {
props: {
articles,
},
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

정적 데이터를 잘 불러와주셨어요!

여기서 생각을 해봐야할것이, getStaticProps는 어느시점의 데이터를 가져오는가 입니다. 이와 비교해서 getServerSideProps는 언제 가져올까요?

베스트게시글이 좋아요 순이라면 실시간으로 바뀌어야할것같은데, 지난주 베스트 게시글이라면 그럴 필요는 또 없는거죠!

때에 따라서 어떤것을 사용해야할지 잘 고민해보시면 좋을 것 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

추가적으로 api error 처리에 대해서도 고민해보시면 좋을 것같아요👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

재미있는 발상입니다👏

Copy link
Collaborator

Choose a reason for hiding this comment

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

util함수를 잘 만들어주셨어요.
그런데 매직넘버가 조금 아쉽네요.
상수화를 시켜주면 더 간결하고 유지보수하기 좋을 것같아요!

const SECONDS_IN_MINUTE = 60;
const SECONDS_IN_HOUR = SECONDS_IN_MINUTE * 60;
const SECONDS_IN_DAY = SECONDS_IN_HOUR * 24;
const SECONDS_IN_WEEK = SECONDS_IN_DAY * 7;
const SECONDS_IN_MONTH = SECONDS_IN_DAY * 30;
const SECONDS_IN_YEAR = SECONDS_IN_DAY * 365;

@lhc0506 lhc0506 merged commit 7a18b61 into codeit-bootcamp-frontend:Next-최민경 Jul 25, 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