[이서라] 스프린트 미션 4#59
Hidden character warning
Conversation
devbini
left a comment
devbini
left a comment
There was a problem hiding this comment.
🎉 고생많으셨습니다!
검색, 정렬, 페이지네이션, 반응형 pageSize 등등...
목록 페이지에서 필요한 기능을 꽤 많이 구현해주셨어요. 처음 React로 API 목록 페이지를 만들 때는 한 기능만 붙여도 신경 쓸 부분이 많은데, 전체적으로 높은 구조 안정성을 보여준 것 같아요!
의존성 배열 관리 역시 React를 초심적으로 사용할 때 가장 어려운 부분인데, 그 역시 큰 문제가 없는 것 같네요 😊
또 추가로, 기능 단위로 컴포넌트를 나누려는 시도도 좋았고, API baseURL을 공통 axios 인스턴스로 만든 것도 좋은 습관이에요. 특히 이런 axios 인스턴스는 실무에서도 채용하는 방법이랍니다! 이렇게 하는 경우, 나중에 인증 헤더나 공통 에러 처리, timeout 같은 설정이 필요해졌을 때 훨씬 관리하기 쉬워집니다.
다만, 코드만 놓고 보면 실제 빌드나 화면에서 바로 문제가 될 수 있는 부분이 몇 개 있어요, import 경로의 대소문자라던지, 이미지 배열 처리나 API 실패 처리 등 실제 사용자가 어떻게 느낄지를 더 생각 해 주시면 좋겠어요.
전체적으로 기능을 많이 구현하신 만큼, 이제는 "예외 상황에서도 화면이 안정적으로 보이는가"를 같이 챙겨보면 더 좋아질 것 같습니다. 앞으로도 많이 기대가 되는 코드였어요!
파이팅입니다 👍
- 아, 추가로.. SVG를 인라인코드로 쭉 작성된 게 많던데.. 가능하면 파일로 분리 해 주세요!ㅎㅎ
유지보수를 넘어 코드 가독성이 떨어지는 원인이 되기도 한답니다!
| import IconYT from '../assets/ic_Youtube.svg'; | ||
| import IconIG from '../assets/ic_Instagram.svg'; |
There was a problem hiding this comment.
🚩 Check Point
조금 위험 해 보이네요.
실제 파일명은 ic_youtube.svg, ic_instagram.svg입니다.
아마.. 제 기억 상 Windows에서는 동작할 수 있지만, Linux 기반 배포 환경에서는 파일을 찾지 못해 빌드가 실패할 수 있어요.
예를 들어, Jenkins 등의 별도 CICD 툴에 들어갔을 때 에러가 날 수도 있답니다.
아무튼, 정리하면-
실제 파일은 소문자이기 때문에 해당 모듈을 찾지 못하고 빌드가 멈출 수 있습니다.
| function EachItem ({ item }) { | ||
| return ( | ||
| <li className={styles.eachItem}> | ||
| <div className={styles.thumbnail}><img src={item.images} alt={item.name} onError={(e) => (e.target.src = defaultImage)}/></div> |
There was a problem hiding this comment.
🚩 Check Point
음.. API의 images는 배열인 것 같아요.
그런데 현재는 배열을 그대로 src에 넣고 있네요.
item.images = [
'https://example.com/a.png',
'https://example.com/b.png',
]
만약 위처럼 값이 들어오게 되면...
<img src="https://example.com/a.png,https://example.com/b.png">
이런 식으로 HTML 구조가 만들어지게 되어버려요.
이건... 정상적인 이미지 주소가 아니기 때문에 썸네일이 깨질 수 있습니다.
이렇게 사용 할 거라면, 첫 번째 이미지를 명확히 꺼내고, 없으면 기본 이미지를 보여주는 방식이 안전해보여요! 😊
| orderBy: 'favorite', | ||
| } | ||
| }); | ||
| const { list } = response.data; |
There was a problem hiding this comment.
🚩 Check Point
현재는 API 요청이 성공한다고 가정하고 바로 response.data를 사용하고 있어요.
예를 들어, 사용자가 페이지에 들어왔는데 네트워크가 잠깐 끊겼다고 해봅시다.
그러면, 지금 구조에서는 axios.get()이 실패하고 이후 코드가 실행되지 않습니다.
그런데 화면에는 "오류가 났다"는 정보가 없어서, 사용자는 상품이 원래 없는 것처럼 이해할 수 있을 것 같아요.
목록 페이지에서는 최소한 로딩/실패 상태를 구분하는 처리가 있으면 좋습니다 😊
너무 어렵게 생각 할 것 없이, try/catch를 이용하기만 해도 충분하지 않을까 싶어요 😊
| gap: 1.2rem; | ||
| } | ||
|
|
||
| @media (max-width:1024) { |
There was a problem hiding this comment.
🚩 Check Point
아마도.. 깜빡하신 것 같아요!
아시다시피- CSS에서 길이 값은 px같은 단위가 항상 필요해요!
| <section className="products-list all"> | ||
| <div className="section-title-header"> | ||
| <h2 className="section-title">판매 중인 상품</h2> | ||
| {isMobile ? ( |
There was a problem hiding this comment.
☕ Thinking
지금 보면 모바일과 데스크톱의 배치가 달라서 JSX를 분기하고 있는 것 같아요.
이런 구분은 자연스럽긴 한데, 검색 form이라던지, 각종 버튼이나 input 구조가 거의 그대로 반복되는 것 같아요.
이렇게 되면... 당연하게도 수정이 필요할 때 양쪽 다 찾아서 수정해줘야 하는 불상사가 생긴답니다. (거기서 누락되는 것도 있을 수 있구요!)
그래서.. 반복되는 부분들은 별도의 컴포넌트로 묶어서 빼 보는 등의 캡슐화는 어떨까 제안 드려봐요 🤔
| const handleSize = useCallback(() => { | ||
| const contentWidth = window.innerWidth; | ||
| if (contentWidth < 480 ){ | ||
| setPageSize(4); | ||
| setIsMobile(true); | ||
| } else if (contentWidth < 1024 ){ | ||
| setPageSize(6); | ||
| setIsMobile(false); | ||
| } else { | ||
| setPageSize(10); | ||
| setIsMobile(false); | ||
| } | ||
| },[]); | ||
|
|
||
| useEffect(() => { | ||
| //처음 | ||
| handleSize(); | ||
| //리사이즈했을때 실행하셈 | ||
| window.addEventListener('resize',handleSize); | ||
| //이벤트리스너 지움 | ||
| return () => window.removeEventListener('resize',handleSize); | ||
| },[handleSize]); |
There was a problem hiding this comment.
☕ Thinking
window.innerWidth로 플랫폼을 구분하는 코드가 페이지에 들어 있는데,
저희가 React를 다루고 있는 만큼, 이런 size 이벤트 관리같은 환경 감지 로직은 별도 Custom Hook으로 만들어 사용할 수 있지 않을까요?!
| setBestItems(list); | ||
| },[pageSize]); | ||
|
|
||
| console.log('페이지사이즈', pageSize); |
There was a problem hiding this comment.
🤔 Thinking
개발중에 log를 찍는 건 당연히 자연스럽고, 저도 구현 중에는 많이 찍긴 합니다만...
실제 서비스 코드에서도 남아있게 되면, 오히려 실제 오류 로그를 확인할 때 방해가 될 수도 있답니다!
| <li onClick={() => handleSortChange('recent')}>최신순</li> | ||
| <li onClick={() => handleSortChange('favorite')}>좋아요순</li> | ||
| </ul> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
☕ Thinking
지금 드롭다운은 div와 li에 onClick을 달아서 동작하고 있네요.
마우스로 클릭하면 동작하지만... 키보드 접근성은 떨어질 수 있습니다.
클릭 가능한 요소는 기본적으로 button을 먼저 고려하는 게 시멘틱 태그를 더 잘 이해하는 부분이 아닐까 싶어요.
또, 커스텀 드롭다운을 직접 구성한다면
aria-expanded같은 속성을 찾아보면 어떨까 싶어요! 😊
항상 이런 고민은 너무 좋은 것 같아요 😊
어떤 요소와 어떤 태그가 UX에 있어 더 좋을지 고민하는 점이요!
|
2 participants
중고마켓 페이지의 핵심 기능(검색, 필터링, 정렬, 페이지네이션)을 구현하고, 공통 레이아웃 및 반응형 디자인(PC/모바일) 적용과 코드 리팩터링을 진행했습니다.
주요 변경 사항
1. 기능 구현 (Features)
2. 코드 구조 개선 (Refactoring)