-
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 #121
The head ref may contain hidden characters: "Next-\uAE40\uD0DC\uD6C8"
[김태훈] sprint9 #121
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.
태훈님 이번 스프린트 미션 고생하셨습니다!
밀려있던 만큼 시작하시기 어려우셨을 것 같은데 대단하네요!
스프린트 미션은 주차별 요구사항을 다 구현하지 않으셔도 하신 만큼 공유해주시면 되는
학습을 위한 도구이므로 앞으로도 부담없이 PR로 만나면 좋겠습니다!
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:
👍
const filteredArticles = articles.filter((article) => | ||
article.title.toLowerCase().includes(searchTerm.toLowerCase()) | ||
); |
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:
https://panda-market-api.vercel.app/docs/#/Article/ListArticles
의 param을 통해서 값을 받아오시는 것을 추천드려요!
그래야 정렬과 키워드가 적용된 값이 내려온답니다~
pages/boards/index.tsx
Outdated
const Board: React.FC = () => { | ||
const [articles, setArticles] = useState<Article[]>([]); | ||
const [error, setError] = useState<string | null>(null); | ||
const [sortOption, setSortOption] = useState<string>("recent"); |
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:
sortOption은 �string보다는 더 좁은 타입인 "recent" | "like" 라고 말할 수 있습니다.
일반적으로는 타입은 좁힐 수 있다면 좁히는 것이 가독성과 코드의 의도를 전달하는 측면에서 더 좋습니다.
const getLinkStyle = (isActive: boolean) => ({ | ||
color: isActive ? "var(--blue)" : undefined, | ||
}); |
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:
해당 함수가 실행되면 CSSProperties
를 반환하니 아래처럼 반환값을 명시해주시는 것이 더 좋을 것 같습니다.
const getLinkStyle = (isActive: boolean) => ({ | |
color: isActive ? "var(--blue)" : undefined, | |
}); | |
const getLinkStyle = (isActive: boolean): CSSProperties['color'] => { | |
return { color: isActive ? "var(--blue)" : undefined }; | |
} |
<nav> | ||
<ul className={styles.navList}> | ||
<li className={styles.navItem}> | ||
<Link href="/community" passHref> |
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:
Link태그의 passHref는 해당 태그의 자식으로 a 태그가 있을경우 적어주는 속성입니다~
그런 경우가 아니라면 빼주시는게 더 좋을 것 같아요~
if (onChange) { | ||
onChange(option); | ||
} |
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:
아래 코드도 동일 동작합니다~
if (onChange) { | |
onChange(option); | |
} | |
onChange?.(option); |
const bestArticles = Array.isArray(articles) | ||
? [...articles].sort((a, b) => b.likeCount - a.likeCount).slice(0, pageSize) | ||
: []; |
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:
위에서 orderBy로 좋아요수로 정렬해서 데이터를 받아오고 있으니 또 정렬하지 않으셔도 될 것 같아요~
const options = [ | ||
{ value: "latest", label: "최신순" }, | ||
{ value: "most_liked", label: "좋아요순" }, | ||
]; | ||
|
||
interface DropdownProps { | ||
value?: { value: string; label: string }; | ||
onChange?: (option: { value: string; label: string }) => 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:
해당 코드를 보면 option의 타입이 반복되고 있으니 아래처럼 하시면 더 좋을 것 같습니다.
const options = [ | |
{ value: "latest", label: "최신순" }, | |
{ value: "most_liked", label: "좋아요순" }, | |
]; | |
interface DropdownProps { | |
value?: { value: string; label: string }; | |
onChange?: (option: { value: string; label: string }) => void; | |
} | |
interface OptionProps { value: string; label: string }; | |
const options: OptionProps[] = [ | |
{ value: "latest", label: "최신순" }, | |
{ value: "most_liked", label: "좋아요순" }, | |
]; | |
interface DropdownProps { | |
value?: OptionProps; | |
onChange?: (option: OptionProps) => void; | |
} |
setIsOpen(false); | ||
}; | ||
|
||
document.body.addEventListener("click", onBodyClick, { capture: true }); |
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:
태훈님 캡쳐링 단계를 감지해야 하는 이유가 무엇일까요?
지금 코드만 봤을때는 click 이벤트 발생시 input에서 클릭된 것인지 아닌지를 따져
option 영역의 노출 여부를 결정하는 것 같습니다.
여기서 { capture: true }
가 없어도 동일 동작할 것 같습니다~
요구사항
기본
심화
검색 기능이 일부 데이터(클라이언트 사이드?)에서만 이루어지고 있어서 수정할 예정