[문지영] sprint5 #197
[문지영] sprint5 #197
Conversation
…ithub-actions [Fix] delete merged branch github action
sinyroom
added
매운맛🔥
…nd/16-Sprint-Mission into React-문지영-sprint5
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
주요 리뷰 포인트
- css-in-js 컨셉에 맞춰 활용도 높이기
- 스타일드 컴포넌트를 일반 컴포넌트와 구분해서 사용하는 팁
- 스크린 사이즈 계산 관련 공통 로직 커스텀 훅 분리 제안
- 리턴문 안쪽 window 전역 객체 접근 관련 피드백
src/api/api.js
Outdated
| @@ -0,0 +1,15 @@ | |||
| import axios from 'axios'; | |||
|
|
|||
| const baseURL = 'https://panda-market-api.vercel.app'; | |||
There was a problem hiding this comment.
보통 base url은 민감한 정보라서, 외부 노출을 막기 위해 환경 변수로 관리하는 편이예요.
아래 아티클 참고해서 환경변수로 관리해볼까요? :)
src/components/Layout/Header.jsx
Outdated
| padding: 0 var(--page-spacing-x-mobile); | ||
|
|
||
| @media (min-width: 768px) { | ||
| padding: 0 var(--page-spacing-x-tablet); | ||
| } | ||
|
|
||
| @media (min-width: 1200px) { | ||
| padding: 0 var(--page-spacing-x-desktop); | ||
| } |
There was a problem hiding this comment.
css-in-js를 사용하셨는데 css vars를 사용하셨네요! 이왕이면 JavaScript 변수나 theme을 이용해 스타일을 동적으로 설정하는 방식으로 변경해 활용도를 높여보는게 어떨까요? 그러면 좀 더 컨셉에 맞게 이 도구를 사용할 수 있을 것 같아요.
우선 아래와 같이 변수를 만들어 직접 활용하는 방식도 가능하지만
const spacingMobile = '16px';
const HeaderContainer = styled.header`
padding: 0 ${spacingMobile};
`;이 방식은 반응형에 대응하기엔 아쉬움이 있으니, theme을 사용하시는게 더 괜찮을 것 같네요.
export const theme = {
spacing: {
mobile: '16px',
tablet: '32px',
desktop: '64px',
}
};const HeaderContainer = styled.header`
padding: 0 ${({ theme }) => theme.spacing.mobile};
@media (min-width: 768px) {
padding: 0 ${({ theme }) => theme.spacing.tablet};
}
@media (min-width: 1200px) {
padding: 0 ${({ theme }) => theme.spacing.desktop};
}
`;| } | ||
| `; | ||
|
|
||
| const HeaderLeft = styled.div` |
There was a problem hiding this comment.
스타일드 컴포넌트는 일반 컴포넌트와 구분하기위해 Styled를 붙여주기도한답니다. (e.g StyledHeaderLeft)
이렇게 일일히 붙여주는게 힘들다면 import하는쪽에서 아래 예시처럼 한꺼번에 named import해서 써주는것도 좋은 방법이고요! 코드를 보다 깔끔하게 유지하기위한 팁중에 하나니까 참고로만 알아두세요 :)
import * as S from '...'
There was a problem hiding this comment.
따로 styled component들만 모아서 파일로 만들기도 하는군요 ! 다음 작업할 때 참고 해보겠습니다 ~
| ${applyFontStyles(FontTypes.BOLD16, ColorTypes.SECONDARY_GRAY_600)}; | ||
|
|
||
| &:hover { | ||
| color: ${({ theme }) => theme.colors[ColorTypes.PRIMARY_100]}; | ||
| } |
There was a problem hiding this comment.
폰트와 컬러 디자인시스템 공용화 시도해두신 점 너무 좋네요 :)
src/components/UI/DropdownList.jsx
Outdated
| return ( | ||
| <DropdownListContainer> | ||
| <StSortButton onClick={() => setIsOpen((prev) => !prev)}> | ||
| {window.innerWidth < 768 ? ( |
There was a problem hiding this comment.
숫자가 하드코딩되어있어 조건을 이해하기 어렵고 관리하기도 어려워질것같아요. 유지보수를 위해 조건식으로 만들고 스크린사이즈를 계산하는 로직을 커스텀 훅으로 분리해보는건 어떨까요? :)
| const getPageSize = () => { | ||
| const width = window.innerWidth; | ||
| if (width < 768) { | ||
| return 4; | ||
| } else if (width < 1024) { | ||
| return 6; | ||
| } else { | ||
| return 10; | ||
| } | ||
| }; |
There was a problem hiding this comment.
여기서도 스크린사이즈를 계산하네요! 특정 로직이 반복되어 쓰인다면 중복을 제거할 필요가 있겠죠?
| useEffect(() => { | ||
| fetchItems(); | ||
|
|
||
| const handleResize = () => { | ||
| setPageSize(getPageSize()); | ||
| }; | ||
| window.addEventListener('resize', handleResize); | ||
| return () => window.removeEventListener('resize', handleResize); | ||
| }, [pageSize, orderBy, currentPage, debouncedKeyword]); |
There was a problem hiding this comment.
의존성 배열중 하나라도 값이 바뀌면 useEffect가 다시 실행되므로 성능 저하가 일어날수있으니, 계산이 불필요하거나 너무 재계산이 빈번하게 일어나는 값은 없는지 판단해볼까요?
|
|
||
| return ( | ||
| <AllProductsContainer> | ||
| {window.innerWidth >= 768 ? ( |
There was a problem hiding this comment.
리턴문안에서 window 전역 객체에 직접 접근하는건 대부분의 경우 좋지않습니다. 지금은 100% CSR 환경이지만 나중에 SSR 환경까지 고려하게된다면 오류가 발생될 가능성이 있고 이때 대응이 어려워져요. 또 이런 코드는 스크린사이즈 관련 계산이 render시마다 실행되고 구조가 중복되기때문에 가독성 측면에서도 좋지않습니다. 리사이징 및 스크린사이즈 처리를 되도록 컴포넌트가 쓰이기 이전에 결정하도록 깔끔하게 분리해보면 좋을것같아요!
There was a problem hiding this comment.
이 파일에서도 AllProducts에 드린 피드백 참고해서 수정해보세요! :)
질문에 대한 답변
검색 api response에서 응답이 이렇게 돌아온다는거죠? api 응답 관련해서는 과제&프로젝트 QnA에 궁금하신점 문의해보시면 될것같네요! |
4 participants
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게