-
Notifications
You must be signed in to change notification settings - Fork 50
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
[Team23] 캐로셀 적용, 모달창 설계 및 구현, 상품 이미지 호버 적용 #30
[Team23] 캐로셀 적용, 모달창 설계 및 구현, 상품 이미지 호버 적용 #30
Conversation
- 캐러셀 구현 - 캐러셀 틀을 잡기 위해 구조 살짝 변경 - 컴포넌트 트리 참고 - products.length가 8의 배수가 아닌 경우를 생각해서 로직을 짰는데 아직 테스트는 안해봄
- 4의 배수가 아닐 때 캐러셀 애니메이션 테스트함 - left 버튼을 누를 때 문제가 있었는데 index 업데이트를 수정하여 고침
- 왼쪽 버튼을 누르면 이전 슬라이드가, 오른쪽 버튼을 누르면 이후 슬라이드가 나오도록 수정
- 아델라의 캐로셀 코드를 demo 브랜치에 머지
- 리렌더링 때문에 깜빡거림
- 공통적으로 사용되는 styled component를 common.jsx에 분리 - 카드에 있는 정보를 모달에 전달
dquote> dquote> class -> struct로 변경 dquote> 변수 var에서 let으로 변경 dquote> bind private 설정, Cold Observalbe 문제 해결 dquote> print문 제거 사용자에게 alertMessage 뜨도록 수정
dquote> dquote> colleciton에 해당하는 protocol선언 (임시) dquote> TablecustomHeaderView 선언
- NetworkManager: Mock 객체 초기화 시에도 URL 외부 주입, 쓰지 않는 URLSession 삭제 - SideDish: CodingKey 추가
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.
안녕하세요, 리뷰를 맡은 스노우 입니다! ☃️
프로젝트를 진행하느라 수고 많으셨습니다! styled-component의 특색을 잘 활용하여 구현하신 것 같습니다.
커스텀 훅의 사용과 모달 구현시 React의 Portals 에 대해 공부해보시면 도움이 많이 될 것 같습니다!
마지막으로 개선이 필요한 부분이 있습니다.
디렉토리 내에 파일들의 구분이 조금 모호합니다. 하나의 예로, components 폴더 내부에 utils 폴더가 있고, 그 안에 유틸 함수로 쓰이거나, 변수를 모아두는 파일들이 있고 또 추가로 기본 컴포넌트로 예상되는 파일들도 혼재되어 있어 조금 혼란스러운 것 같습니다. 컴포넌트의 유틸과 전체 유틸을 분리하거나 다른 네이밍을 사용하는 것이 좋을 것 같습니다. 또한, 전체적인 파일 및 컴포넌트 구성에 대해 다시 한 번 점검이 필요할 것 같습니다!
이외에는 깔끔하게 재사용을 고려하여 코드를 잘 작성하셨습니다. 👍
|
||
useEffect(() => { | ||
fetch( | ||
'https://h3rb9c0ugl.execute-api.ap-northeast-2.amazonaws.com/develop/baminchan/main' |
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.
자주 쓰는 API를 상수로 지정하거나 객체 형태로 모아서 쓰는 건 어떨까요 ?
!distance && setRightDisabled(true); | ||
nextIndex < products.length && setLeftDisabled(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.
동일한 로직에 좌우로만 차이가 있는데 이를 통합한 하나의 함수로 짜보는 건 어떨까요 ?
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.
넵! 수정하겠습니다
const [currentX, setX] = useState(0); | ||
const [currentIndex, setCurrentIndex] = useState(4); | ||
const [rightDisabled, setLeftDisabled] = useState(false); | ||
const [leftDisabled, setRightDisabled] = useState(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.
left right disabled를 state로 갖지 않고, 슬라이드의 Index와 length로 충분히 처리할 수 있을 것 같습니다!
이런 방향성도 고려해보세요!
) | ||
.then((res) => res.json()) | ||
.then((response) => { | ||
response.body.forEach((e) => { |
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.
e같은 추상적인 변수명보다는 구체적인 변수명이 좋습니다!
@@ -24,4 +24,27 @@ const CardList = styled.ul` | |||
justify-content: center; | |||
`; | |||
|
|||
export { Button, CenterContainer, SectionTitle, CardList }; | |||
const StyledTitle = styled.div` |
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.
기본 UI 를 모아서 쓰는 방식 좋군요!
오늘 한 일
새벽배송 | 전국택배
출력)앞으로 할 일
컴포넌트 트리
오늘도 감사합니다 리뷰어님~ 😊