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

[Feat/#120] 메인 페이지 Carousel 기능 추가 #121

Merged
merged 5 commits into from
Jul 14, 2024

Conversation

sinji2102
Copy link
Contributor

@sinji2102 sinji2102 commented Jul 12, 2024

📌 관련 이슈번호

🎟️ PR 유형

어떤 변경 사항이 있나요?

  • 새 기능 추가
  • 버그 수정
  • CSS 등 사용자 UI 디자인 변경
  • 리팩토링

✅ Key Changes

  • mainPage에 있는 캐러샐 구현!!

📢 To Reviewers

  • 메인 페이지 브랜치를 들고온 거라서 main > components > carousel 파일만 보면 됩니다~

📸 스크린샷

-Chrome2024-07-1303-58-25-ezgif com-video-to-gif-converter
현재 gif는 5초인데 디자인 측 요청으로 3초로 변경되었습니다!

@sinji2102 sinji2102 linked an issue Jul 12, 2024 that may be closed by this pull request
1 task
Copy link

PR 작성하느라 고생 많았어요!! 라벨 잘 지정되었는지 확인 한 번 해 주기 🫶

@github-actions github-actions bot added the ✨ FEAT 기능 구현 label Jul 12, 2024
@github-actions github-actions bot requested review from imddoy, ocahs9 and pepperdad July 12, 2024 19:02
Copy link
Contributor

@ocahs9 ocahs9 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 +29 to +36
const moveToNthSlide = (index: number) => {
setTimeout(() => {
setCurrIndex(index);
if (carouselRef.current !== null) {
carouselRef.current.style.transition = "";
}
}, 500);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 일정 시간을 준 뒤에 우선 인덱스를 옮겨보고, 해당 인덱스의 위치가 존재하지 않으면 이동하는 애니메이션을 존재하지 않도록 하는 센스 좋아요 ~~!

Comment on lines +39 to +47
useEffect(() => {
if (carouselList.length !== 0) {
const startData = carouselList[0];
const endData = carouselList[carouselList.length - 1];
const newList = [endData, ...carouselList, startData];

setCurrList(newList);
}
}, [carouselList]);
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 새로운 배열을 연결하는 방식을 채택하셨네요! 그걸 위해 마지막 데이터와 시작 데이터를 양쪽에 추가해줘서 좀 더 자연스럽게 만들어준 것도 좋아요 ~!

// idx 변경되면 위치 이동
useEffect(() => {
if (carouselRef.current !== null) {
carouselRef.current.style.transform = `translateX(-${currIndex}00%)`;
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) ${currIndex}00% 귀엽네요! 가독성도 좋아요!

Comment on lines +72 to +73
return () => {
clearInterval(time);
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 언마운트될 때 클린업 함수 작성하는 센스 좋은데요? 사이드 이펙트 방지하는 것 같고 좋네요!


list-style: none;

img {
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 까먹고 있었는데 button 요소 의 하위 요소로 img 요소가 왔을 떄의 스타일 정의 방식은 오랜만에 보내요!
딱히 코드의 내용이 없을 때 활용하면 좋긴 하겠네요 ~!

<S.InfoLayout>
<S.InfoBtn
onClick={() => {
window.open(privacy, "개인정보처리방침", "noopener");
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 오 ! 주소는 privacy 변수로 담아 넘겨주고, 두번째 인자 windowName 은 새 창의 name 속성으로 사용된다고 하더라구요. 그래서 만약 이미 같은 name의 창이 열려있으면 해당 창으로 이동하게 만들어주는 기능을 해준다고 합니다!

마지막으로, 세번째 인자인 windowFeatures는 보안 기능이라고 합니다 ! noopener를 줄 경우 새 창이 원본 창의 window.opener 속성을 제어할 수 없게 만든다고 하네요. XSS에 대해 한번 설명한 적이 있는데, 악성 스크립트가 삽입되어 있는 사이트나, 애초에 악의적인 목적으로 만들어진 사이트에서 원본 창을 제어하여 쿠키를 유출하는 경우를 방지하기 위함인 것 같습니다 . 참고하세요~!

Copy link
Member

@pepperdad pepperdad left a comment

Choose a reason for hiding this comment

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

GOOD

// 서버에서 이미지 url 받아오기
const ImgList = [carouselImg, carouselImg, carouselImg, carouselImg, carouselImg];

const Carousel = (promotionList: PromotionComponentProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const Carousel = (promotionList: PromotionComponentProps) => {
const Carousel = ({ promotionList }: PromotionComponentProps) => {

Copy link
Member

@pepperdad pepperdad left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@imddoy imddoy 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 +4 to +6

import carouselImg from "../../../../assets/images/banner_roll.png";
interface PromotionProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

p5) 파일 경로 이렇게 작성하신 이유가 있을까여??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

서버에서 보내주는 이미지를 삽입해야 해서 임시로 넣은 이미지입니다!

@sinji2102 sinji2102 merged commit c152bad into develop Jul 14, 2024
1 check failed
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ FEAT 기능 구현
Projects
Status: 🎉Done
Development

Successfully merging this pull request may close these issues.

[ Feat ] 메인 페이지 Carousel 추가
4 participants