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

[신윤하] sprint11 #325

Conversation

ayoooyh
Copy link
Collaborator

@ayoooyh ayoooyh commented Aug 26, 2024

요구사항

기본

  • [x]
  • []
  • []

심화

  • [x]
  • []

주요 변경사항

스크린샷

image

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@ayoooyh ayoooyh requested a review from arthurkimdev August 26, 2024 03:38
@ayoooyh ayoooyh self-assigned this Aug 26, 2024
@ayoooyh ayoooyh added the 미완성🫠 죄송합니다.. label Aug 26, 2024
@ayoooyh ayoooyh changed the title Next 신윤하 sprint11 [신윤하] sprint11 Aug 26, 2024
Copy link
Collaborator

@arthurkimdev arthurkimdev left a comment

Choose a reason for hiding this comment

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

마지막 과제 제출까지 수고하셨습니다! 👍

Comment on lines +75 to +90
.addImage {
width: 282px;
height: 282px;
border-radius: 12px;
background: #f3f4f6;

font-size: 16px;
font-weight: 400;
line-height: 26px;
text-align: left;

color: #9ca3af;
align-content: center;
/* justify-content: center; */
gap: 12px;
}
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 +24 to +41
<div
className={styles.titleInput}
id="title"
value={title}
onChange={(e) => setTitle(e.target.value)}
>
제목을 입력해주세요
</div>
<span className={styles.titleFont}>*내용</span>
<div
className={styles.contentInput}
id="content"
value={content}
onChange={(e) => setContent(e.target.value)}
isTextArea
>
내용을 입력해주세요
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

웹 접근성을 위해 입력과 관련된 사항은 아래처럼 <input>, <textarea> 를 사용해주셔야 해요. div 사용 시, 키보드 접근성과 스크린 리더 호환성이 떨어집니다.

<label htmlFor="title" className={styles.titleFont}>*제목</label>
<input
  type="text"
  id="title"
  className={styles.titleInput}
  value={title}
  onChange={(e) => setTitle(e.target.value)}
  placeholder="제목을 입력해주세요"
/>

<label htmlFor="content" className={styles.titleFont}>*내용</label>
<textarea
  id="content"
  className={styles.contentInput}
  value={content}
  onChange={(e) => setContent(e.target.value)}
  placeholder="내용을 입력해주세요"
/>
 

Comment on lines +8 to +11
.Image {
width: 300;
height: 300;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

다른 곳에서는 선무 camelCase 였는데, 갑자기 여기서 PascalCase가 되는 것 같아요. 통일성을 위해 아래처럼 변경되면 좋겠습니다.

.image {
  width: 300;
  height: 300;
}

Comment on lines +39 to +50
const useViewport = () => {
const [width, setWidth] = useState(0);

useEffect(() => {
const handleWindowResize = () => setWidth(window.innerWidth);
handleWindowResize();
window.addEventListener("resize", handleWindowResize);
return () => window.removeEventListener("resize", handleWindowResize);
}, []);

return width;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 함수 경우 resize 실행 될 때 최적화가 필요합니다. 이럴 땐 연속적인 이벤트가 발생 시 마지막 이벤트만 처리하는 debounce 개념을 통해 해결할 수 있어요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아래처럼 리팩토링 할 경우, 불필요한 연산을 줄이고 성능을 향상 시킬 수 있습니다. lodash/debounce 라이브러리 크기가 무겁다면, just-debounce 사용해도 무방해요.

import debounce from 'lodash/debounce';

function useViewport() {
  const [width, setWidth] = useState(() => 
    typeof window !== 'undefined' ? window.innerWidth : 0
  );

  const handleWindowResize = useCallback(() => {
    const newWidth = window.innerWidth;
    if (newWidth !== width) {
      setWidth(newWidth);
    }
  }, [width]);

  useEffect(() => {
    if (typeof window === 'undefined') return;

    const debouncedHandleResize = debounce(handleWindowResize, 150);

    window.addEventListener('resize', debouncedHandleResize);
    
    return () => {
      debouncedHandleResize.cancel(); // Lodash debounce의 cancel 메서드
      window.removeEventListener('resize', debouncedHandleResize);
    };
  }, [handleWindowResize]);

  return width;
}

export default useViewport;

Comment on lines +85 to +111
<div key={article.id} className={styles.bestArticleCardContainer}>
<Image
unoptimized={true}
src="/assets/images/bestBadge.png"
width={102}
height={30}
alt="베스트 뱃지"
/>

<div className={styles.bestArticleCard}>
<span className={styles.cardTitle}>{article.title}</span>
<Image
className={styles.bestArticleImage}
src={article.image}
width={72}
height={72}
alt={`${article.title} 이미지`}
/>
</div>
<div className={styles.bestArticleFooter}>
<div className={styles.bestArticleInform}>
<div>{article.writer.nickname}</div>
<div>{article.likeCount}</div>
</div>
<div>{format(new Date(article.createdAt), "yyyy. MM. dd")}</div>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 map을 사용하는 부분은 별도의 컴포넌트로 분리하는것이 좋아요. jsx가 줄어들면서 현재 보고 있는 컴포넌트에 더 집중할 수 있고, map을 사용한다는말은, 다른 곳에서도 얼마든지 재사용이 가능하거든요.
아래처럼 구현하는 방법도 있어요.

function ArticleCard({ article }: { article: Article }) {
  return (
    <div className={styles.bestArticleCardContainer}>
      <Image
        src="/assets/images/bestBadge.png"
        width={102}
        height={30}
        alt="베스트 뱃지"
      />
      <div className={styles.bestArticleCard}>
        <span className={styles.cardTitle}>{article.title}</span>
        <Image
          className={styles.bestArticleImage}
          src={article.image}
          width={72}
          height={72}
          alt={`${article.title} 이미지`}
        />
      </div>
      <div className={styles.bestArticleFooter}>
        <div className={styles.bestArticleInform}>
          <div>{article.writer.nickname}</div>
          <div>{article.likeCount}</div>
        </div>
        <div>{format(new Date(article.createdAt), "yyyy. MM. dd")}</div>
      </div>
    </div>
  );
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

그럼 결국 이렇게 짧게 변경해볼 수 있겟죠?

...
 return (
    <div>
      <div className={styles.bestArticleHeader}>
        <span className={styles.bestArticleTitle}>베스트 게시글</span>
      </div>
      <div className={styles.allContainer}>
        {articles.map((article) => (
          <ArticleCard key={article.id} article={article} />
        ))}
      </div>
    </div>
  );
}

Comment on lines +13 to 16
<SearchForm />

<form className={styles.searchForm} onSubmit={handleSubmit}>
<SearchIcon alt=" " />
<input
className={styles.searchInput}
value={value}
placeholder="검색할 상품을 입력해주세요"
onChange={handleChange}
/>
</form>
<Articles />
</>
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 +54 to +75
useEffect(() => {
if (viewportWidth === 0) return;

const newPageSize = getPageSize(viewportWidth);

if (newPageSize !== pageSize) {
setPageSize(newPageSize);

const axiosBestArticles = async (size: number) => {
try {
const response = await axios.get<ArticleListResponse>(
`https://panda-market-api.vercel.app/articles?orderBy=like&pageSize=${size}`
);
setArticles(response.data.list);
} catch (error) {
console.error("Failed to fetch best articles:", error);
}
};

axiosBestArticles(newPageSize);
}
}, [viewportWidth, pageSize]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

API 요청하는 부분은 별도의 커스텀 훅으로 빼서 사용하면 더 좋겠어요. 재사용이 될 수 있거든요. 뿐만 아니라 공통 에러 처리도 함께 작성해볼 수 있구요.

@arthurkimdev arthurkimdev merged commit dce4972 into codeit-bootcamp-frontend:Next-신윤하 Aug 27, 2024
1 check failed
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.

4 participants