-
Notifications
You must be signed in to change notification settings - Fork 21
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 #127
The head ref may contain hidden characters: "Next-\uC774\uC218\uC9C0-sprint10"
[이수지] sprint10 #127
Conversation
…tch. 에러로 인한 설정 변경
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.
수지님 이번 스프린트 미션 고생하셨습니다~
동일 사항에 대해서는 한번만 코멘트를 드리니 리팩토링하실때 비슷한 사항도 찾아서 같이 적용해보시면 더 좋을 것 같습니다~
|
||
useEffect(() => { | ||
const fetchArticles = async () => { | ||
let url = `https://panda-market-api.vercel.app/articles?orderBy=${orderBy}`; |
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.
P3:
공통으로 사용되는 기본 주소는 변수에 저장해두고 쓰셔도 좋을 것 같아요~
let url = `https://panda-market-api.vercel.app/articles?orderBy=${orderBy}`; | |
let url = `${BASE_URL}/articles?orderBy=${orderBy}`; |
const handleInputChange = (e: ChangeEvent<HTMLTextAreaElement>) => { | ||
setComment(e.target.value); | ||
}; |
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.
P2:
이렇게 인풋값이 입력될때마다 state를 바꿔주는 경우 debounce를 통해 이벤트를 묶어 실행해주시는 것이 좋습니다.
그렇게 되면 리렌더링되는 횟수를 줄일 수 있습니다.
interface ItemProfileSectionProps { | ||
product: Product; | ||
} |
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.
P3:
product으로 묶어 전달 받기보다 images, name, price 등으로 받는 게 더 좋아보여요~
product 객체로 받으면 꼭 다른 값도 받기위해서 묶어서 받는 것처럼 보이니, 필요한 값만 전달 받으시면 더 좋을 것 같아요
: // 참고: 요구사항에는 없었지만 항상 Empty State UI 구현하는 걸 잊지 마세요! Empty State을 재사용 가능한 컴포넌트로 만들었어요. | ||
// 키워드가 입력되지 않은 상태에서 검색 시 Empty 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.
P3:
좋은 주석이 많네요~ empty UI, skeleton UI 등을 구현하는것이 늘 더 좋습니다~
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.
P3:
많은 주석과 여러개의 컴포넌트, styledComponent 로 인해 파일의 가독성이 떨어집니다.
분리하시면 더 좋을 것 같아요~
const getPageSize = (width: number) => { | ||
if (width < 768) { | ||
return 4; // Mobile viewport | ||
} else if (width < 1280) { | ||
return 6; // Tablet viewport | ||
} else { | ||
return 10; // Desktop viewport | ||
} | ||
}; |
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.
P3:
해당 함수의 반환값이 number가 아닌 4 | 6 | 10 이므로 pageSize에서도 이를 명시해주시면 더 좋습니다.
type PageSizeType = ReturnType<typeof getPageSize>;
const [pageSize, setPageSize] = useState<PageSizeType | null>(null);
// Next.js에서는 next/link의 Link 컴포넌트를 사용해 주세요. | ||
// 참고: Next.js 버전 13부터는 Link 자체가 anchor 태그의 역할을 해요. Link 요소 내에 <a> 태그가 중첩되어 있으면 hydration 오류가 발생하니 주의해 주세요. |
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.
P3:
학습에 좋은 주석이네요 👍
`; | ||
|
||
interface DropdownMenuProps { | ||
onSortSelection: (sortOption: any) => void; |
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.
P2:
any 보다는 타입을 명시해주시면 좋겠습니다.
여기서는 {key: string; label: string}으로 아래의 sortOptions 와 동일한 타입이므로 따로 정의해서 사용하시면 더 좋겠네요.
interface DropdownOptionProps { key: string; label: string }
interface DropdownMenuProps {
onSortSelection: (sortOption: DropdownOptionProps) => void;
sortOptions: DropdownOptionProps[];
}
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.
P3:
파일 길이가 250줄 가까이되니 가독성이 떨어지네요~
이미지 업로드같은 내부 로직들을 custom hook으로 분리하면 좋을 것 같아요.
writer: { | ||
image: string; | ||
nickname: string; | ||
id: number; | ||
}; |
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.
P2:
writer가 해당 파일과 articleTypes에서 반복적으로 사용되는 것 같으니 공용으로 타입하나 정의해서 쓰시면 더 좋을 것 같습니다.
export interface WriterProps {
image: string;
nickname: string;
id: number;
};
export interface ProductComment {
writer: WriterProps;
}
export interface Article {
writer: Omit<WriterProps, 'image'>;
}
요구사항
기본
상품 등록 페이지
상품 상세 페이지
심화
상품 등록 페이지
주요 변경사항
스크린샷
멘토에게