-
Notifications
You must be signed in to change notification settings - Fork 2
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
🎉 이미지 슬라이더 구현 #22
🎉 이미지 슬라이더 구현 #22
Conversation
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 [currentIndex, setCurrentIndex] = useState(1) | ||
const [coordinate, setCoordinate] = useState({ start: 0, end: 0 }) | ||
const [style, setStyle] = useState({ | ||
transform: `translateX(-${currentIndex}00%)`, |
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.
혹시 currentIndex * 100
같은 연산으로 처리하는거 어떨까요??
} | ||
}, [currentIndex, imageList.length]) | ||
|
||
return ( |
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.
컴포넌트가 이미지, 버튼, 하단 인디케이터 세 가지로 분리되는 것 같은데
인디케이터 정도라도 따로 외부에서 빼서 만들면 조금 코드 읽기 수월해질 것 같습니다!
useEffect(() => { | ||
if (!autoSlide) return | ||
const slideInterval = setInterval(goNextSlide, autoSlideInterval) | ||
return () => clearInterval(slideInterval) | ||
}, [autoSlide, autoSlideInterval, currentIndex, goNextSlide]) | ||
|
||
useEffect(() => { | ||
if (currentIndex === 0) { | ||
setCurrentIndex(imageList.length - 2) | ||
setTimeout(function () { | ||
setStyle({ | ||
transform: `translateX(-${imageList.length - 2}00%)`, | ||
transition: '0ms', | ||
}) | ||
}, 400) | ||
} |
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.
혹시 핸들러가 아니라 effect로 각 케이스를 따로 주어야 하는 이유가 있나요? (제가 제대로 이해를 못 한거 같아서) 궁금해서 남깁니다.
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.
물론 저희 기획 상 이미지가 있는 가정이지만, 해당 슬라이더는 그와 별개로 추상화되어야 하므로 이미지가 없을 때 기본 이미지가 있는게 좋을 것 같습니다! CLS 때문이라도 그게 좋을 것 같아요
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 end = e.changedTouches[0].pageX | ||
const distance = Math.abs(coordinate.start - end) | ||
|
||
if (coordinate.start > end && distance > 2) { |
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.
2라는 특정 숫자가 많이 나오는데, 코드에 대한 자세한 이해는 못했지만 매직넘버인 것 같아 상수화 하시면 좋을 것 같습니다.
useEffect(() => { | ||
if (currentIndex === 0) { | ||
setCurrentIndex(imageList.length - 2) | ||
setTimeout(function () { | ||
setStyle({ | ||
transform: `translateX(-${imageList.length - 2}00%)`, | ||
transition: '0ms', | ||
}) | ||
}, 400) |
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 SLIDE_TRANSITION_DELAY = 400; 으로 정의해줘도 좋을 것 같습니다.
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.
actions 결과를 다시 확인해주세요. -자동으로 작성됨-
- 목적
관련 티켓 번호: 87
- 주요 변경 사항
기타 사항 (선택)
- 스크린샷 (선택)