-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: 메인페이지 개발 #109
feat: 메인페이지 개발 #109
Conversation
astro 사용을 위해 astro 전용 플러그인 별도 추가
commit convention, pre-commit
인증 모듈 개발 시 수정 예정
<button | ||
disabled={page === i} | ||
id="pagination" | ||
onClick={() => handleClick(i)} |
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.
큰 차이는 아니지만 매번 이렇게 함수를 동적으로 생성하는 것 보다
handleClick 함수를 고차 함수로 만들어서 사용하면 좀 더 가볍게 만들 수 있어요
handleClick = (page: number) => () => {
setPage(page);
}
onClick={handleClick(i)}
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.
큰 차이는 아니지만 매번 이렇게 함수를 동적으로 생성하는 것 보다 handleClick 함수를 고차 함수로 만들어서 사용하면 좀 더 가볍게 만들 수 있어요
handleClick = (page: number) => () => { setPage(page); } onClick={handleClick(i)}
리뷰보다가 궁금한것이 있어서 @KimHunJin 헌진님께 질문드립니다!
작성하신 형태로 많이 쓰고 있는데 이게 고차함수인지는 처음 알았네요 ㅎㅎ
혹시 왜 가벼워지는 지에 대해서 공부할 수 있게 어떤 키워드를 찾으면 좋을 지 알려주실 수 있을까요?
(gpt에 물어보니 익명함수라서 매번 새롭게 할당된다?)
그리고 onClick={handleClick(i)}를 넘기면 함수호출형이라서 바로 실행이 되지 않을까요?
저렇게 함수 호출형을 넘겨줬을 때 바로 실행되던 순간들이 있어서 여쭤봅니다!
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.
아하 지금보니 다시 이해했습니다. 바로 실행되지 않습니다.......
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.
형태가 다르군요.......다시 보니 이해했습니다.
질문 무시해주셔도 됩니다!
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.
@headring 님
넵 렌더링을 진행할 때 마다 매번 함수를 새로 생성하기 때문에 옛날에는 성능상 좋지 않아 지양하긴 했었어요.
사실 해당 함수 자체가 고차함수기 때문에 렌더링 할 때마다 함수를 리턴 받는건 동일하긴 한데, 컨벤션 처럼 지키려고 하기도 해서 렌더링 시점에 함수를 생성하는 방식을 가급적 피하는 편입니다!
요즘은 뭐 워낙 개선이 많이 돼서 성능적으로 큰 차이는 없을거지만요 :)
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.
수고하셨습니다!
아래코드 같은 복잡한 css 방식?? 도 잘 사용하시고 transition 이나 애니메이션 효과도 잘 주셨네요 👍
[`${pageButton}.active > &`]: {
offset: number; | ||
} | ||
|
||
const Pagination = ({ totalPage, page, setPage, offset = 5 }: Props) => { |
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.
Pagination.tsx
파일에는 UI위주로 작성하고 로직은 usePagination.ts
과 같은 커스텀 훅으로 만들어도 좋을 것 같아요! 이건 취향 차이여서 참고만 해주세요 ㅎㅎ
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.
말씀해주신대로 뷰 영역과 로직을 분리하여 관심사를 명확히 구분하는게 좋을 것 같네요!
: data.articles.map((v, i) => ( | ||
<Article | ||
key={`article-${i}`} | ||
favorited={v.favorited} | ||
favoritesCounts={v.favoritesCount} | ||
imgSrc={v.author.image} | ||
authorName={v.author.username} | ||
createDate={v.createdAt} | ||
title={v.title} | ||
description={v.description} | ||
tagComponent={<TagList tagList={v.tagList} />} | ||
></Article> | ||
))} |
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.
v
대신 article
같이 의미있는 변수명을 사용해도 좋을 것 같습니다 !
- data.articles.map((v, i) => (`
+ data.articles.map((article, i) => (`
저는 게시글 리스트 컴포넌트에 map()
을 사용해서 게시글 컴포넌트를 나타낼 때 아래와 같이 data를 한번에 넘기고 자식 컴포넌트에서 구조분해 할당으로 데이터를 가져왔었는데 이 방식에 대해서 사람들이 어떤방식을 더 선호하는지 토론글을 봤었는데 어느 방식을 사람들이 더 선호했었는지 기억이 안나네요 ㅎㅎ;
// ArticleList component
{articleList.map((article,index)=> (
<Article article={article} />
)}
// Article component
const Article = ({ article }) => {
const { username, content, createdAt, description} = article;
return (
<div>{username}</div>
<div>{content}</div>
<div>{createdAt}</div>
<div>{createddescription}</div>
or 아래와 같이 표현도 가능합니다. 참고만 해주세요 !
: data.articles.map((v, i) => (
<Article {...v} />
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.
처음엔 최대한 추상화하여 만들어보고자 하였으나 feature 단위의 컴포넌트다 보니 Article 도메인에 종속된 컴포넌트가 되어버렸네요 도메인에 종속되었으니 article 자체를 전달받아 사용하는 것이 더 깔끔할 것 같네요! 꼼꼼한 확인 감사합니다
return ( | ||
<> | ||
{tagList.map((v, i) => ( | ||
<Label key={`tag-${i}`} text={v} /> |
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.
key 값은 항상 unique
하게 설정하시네요 ! 👍👍
여기도 v
대신 tag
로 작성하셔도 좋을 것 같아요
try { | ||
setLoading(true); | ||
const response = await httpClient.get( |
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.
통신이 실패해서 catch문을 탈 수 있으니setLoading은 try문 위에 있어야 하지 않나요 ??
- try {
- setLoading(true);
+ setLoading(true);
+ try {
그리고 loading 상태값 컨벤션이 맞춰지면 좋을 것 같습니다~
- const [isLoading, setLoading] = useState(false);
+ const [isLoading, setIsLoading] = useState(false);
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.
통신이 실패해서 catch문을 탈 수 있으니setLoading은 try문 위에 있어야 하지 않나요 ??
제 생각에 해당 함수에서 setLoading의 위치로 인한 문제는 없을거 같아요!
통신을 타기 전에 loading을 진행하기 때문에 문제되지는 않을거 같네요.
다만, 관심사를 명확히 분리한다면 리뷰처럼 try 밖으로 빼는게 좋을거 같습니다~
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.
작성하신 스타일이나 Skeleton 코드 정말 잘 봤습니다.
참고 해보겠습니다 :)
src/components/tab/Tab.tsx
Outdated
|
||
return ( | ||
<div className={container}> | ||
<ul ref={elementRef} className={`${tabContainer} ${containerStyle} ${isExpanded ? "expanded" : ""}`}> |
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.
참고만 해주시면 될거 같아요~
저는 classname이 길어지고, 여러 조건이 생기는 경우 개인적으로 밖으로 빼서 관리하는 편입니다!
const classnames = () => {
const cn = [tabContainer, containerStyle];
if (isExpanded) {
cn.push('expanded');
}
return cn.join(' ');
}
import HeaderLink from "./HeaderLink.astro"; | ||
|
||
import { Image } from "astro:assets"; | ||
--- |
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.
1번과 8번 라인에 대시 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.
아스트로 컴포넌트의 렌더링 단계에서 한번만 실행되는 스크립트 영역입니다!
dom 렌더링 이전에 동작하여 window 객체 접근이 불가능하고 '---' 내부에서 한번 계산된 동적 값은 변경할 수 없는 값이 되어버리는 특징이 있습니다
} | ||
} | ||
|
||
export default new HttpClient(); |
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.
오 여기서는 클래스 문법을 쓰셨네요
함수로 통일 해보는 것도 좋은 경험이 될거 같습니다~
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.
네! fetch 영역을 intercepter 까지 추가하여 리팩토링 해보려고 합니다!
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.
새로운 코드 잘 보고 갑니다!
코드마다 다양한 방식들을 볼 수 있어서 흥미롭네요
📌 이슈 링크
📖 작업 배경
🛠️ 구현 내용
💡 참고사항
🖼️ 스크린샷