-
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
[한수진] Sprint9 #120
The head ref may contain hidden characters: "Next-\uD55C\uC218\uC9C4-sprint9"
[한수진] Sprint9 #120
Conversation
[Fix] delete merged branch github action
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.
수진님 이번 스프린트 미션 고생하셨습니다~
제가 달아드린 코멘트들 중 중복되는 사항의 경우 한번만 코멘트를 달아두었으니 다른 케이스도 같이 찾아보시면 좋겠습니다.
또한 타입이 명시되지 않은 부분이 있으니 다음에는 타입 정의, 추가를 우선적으로 하시는 것을 추천드립니다~
lib/axios.js
Outdated
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:
👍
styles/ArticleList.module.css
Outdated
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:
개인적으로 코드리뷰 드리기엔 styles
폴더에 css 파일들이 몰려있는것이 좋지만
프로젝트 자체로 보면 관련있는 컴포넌트 근처에 해당 컴포넌트의 css 파일이 있는것이 co-location 원칙에 더 부합하고
더 합리적일 것 같아요~
그렇게 한 폴더안에 관련된 파일들이 존재하면 프로젝트 구조를 모르는 사람도 관련있는 것끼리 묶여있어 파악하기도 더 쉽습니다.
import styles from '@/styles/Search.module.css'; | ||
|
||
export default function Search() { | ||
const [article, setArticle] = 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.
P2:
타입을 명시해주시면 좋을 것 같아요.
이름도 배열임을 알 수 있게 해주시면 더 좋겠습니다~
const [article, setArticle] = useState([]); | |
const [article, setArticle] = useState<ArticleProps[]>([]); |
useEffect(() => { | ||
const handleResize = () => { | ||
setDisplayedArticles(getDisplayedArticles(articleList)); | ||
}; | ||
|
||
window.addEventListener('resize', handleResize); | ||
|
||
return () => { | ||
window.removeEventListener('resize', handleResize); | ||
}; | ||
}, [articleList]); |
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:
resize 이벤트를 감지해서 display되는 article 수를 변경해주는 로직들을 모아 custom hook으로 분리하시면
더 좋을 것 같습니다.
const sortedArticles = res.data.list.sort( | ||
(a, b) => b.likeCount - a.likeCount | ||
); |
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.
P1:
프론트에서 정렬을 바꾸시는건 의미가 없습니다~
이렇게 하는 것이 서버에서 정렬해서 주는 것과 같은 값이 나오려면, 서버에 있는 모든 데이터를 가지고와야합니다.
(위에서 1000개를 가지고 오긴 하셨습니다만 이게 전부인지는 확실하지 않습니다.)
article을 가지고 오는 api param중 orderBy를 통해서 정렬이 된 값을 가지고 와주세요~
if (width < 600) { | ||
return articles.slice(0, 1); | ||
} else if (width < 900) { | ||
return articles.slice(0, 2); | ||
} else { | ||
return articles.slice(0, 3); | ||
} |
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:
각 조건 일치시 early return 하므로 아래 코드도 동일 동작합니다.
if (width < 600) { | |
return articles.slice(0, 1); | |
} else if (width < 900) { | |
return articles.slice(0, 2); | |
} else { | |
return articles.slice(0, 3); | |
} | |
if (width < 600) { | |
return articles.slice(0, 1); | |
} | |
if (width < 900) { | |
return articles.slice(0, 2); | |
} | |
return articles.slice(0, 3); |
const [behindList, setBehindList] = useState([]); | ||
const [likedArticles, setLikedArticles] = useState([]); | ||
const [newArticles, setNewArticles] = useState([]); | ||
const [sortOption, setSortOption] = useState('new'); |
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:
원시값이 아닌 이런 친구들도 어떤 값인지 타입을 명시해주시면 더 좋습니다.
className={styles.optionValue} | ||
key={option.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:
key는 필수값으므로 파악하기 쉽게 최상단에 두는 것을 추천드려요.
className={styles.optionValue} | |
key={option.value} | |
key={option.value} | |
className={styles.optionValue} |
요구사항
기본
심화
주요 변경사항
-아래쪽 게시물 불러오기 부분에 더보기 버튼을 추가하여 버튼으로 추가할 수 있게 해두었습니다.
스크린샷
멘토에게