Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @dasosann, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 렌딩 페이지의 사용자 경험을 개선하기 위해 이미지 형식을 최적화하고 캐러셀에 터치 스와이프 기능을 도입합니다. 또한, UI 구조를 재정비하여 이미지와 텍스트 콘텐츠의 표시 방식을 분리함으로써 시각적 일관성과 반응성을 향상했습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 새로운 랜딩 페이지 디자인을 적용하기 위해 캐러셀 로직을 리팩토링하는 변경 사항을 포함하고 있습니다. 이미지와 텍스트 애니메이션을 분리하여 더 동적인 사용자 경험을 제공하려는 의도가 좋아 보입니다.
코드 리뷰에서는 몇 가지 개선점을 제안했습니다.
- 성능 개선:
handleTouchMove에서 불필요한 리렌더링을 유발하는useState대신useRef를 사용하도록 제안했습니다. - 버그 수정: 캐러셀 인디케이터가 중복으로 렌더링되는 문제를 수정하고, 텍스트와 함께 슬라이드되지 않도록 구조를 변경하는 것을 제안했습니다.
- 가독성 향상: 하드코딩된 값을 상수로 추출하여 코드의 명확성과 유지보수성을 높이는 방안을 제안했습니다.
전반적으로 좋은 방향의 개선이며, 제안된 사항들을 반영하면 더 완성도 높은 코드가 될 것입니다.
| const [touchStart, setTouchStart] = useState(0); | ||
| const [touchEnd, setTouchEnd] = useState(0); |
There was a problem hiding this comment.
touchStart와 touchEnd 값을 useState로 관리하면 handleTouchMove가 호출될 때마다 불필요한 리렌더링이 발생하여 성능이 저하될 수 있습니다. 터치 이동 중 좌표는 렌더링에 직접적인 영향을 주지 않으므로, useRef를 사용하여 값을 관리하는 것이 더 효율적입니다. useRef로 변경하고 관련 핸들러 함수들도 수정해주세요.
// ...
import { useEffect, useState, useRef } from 'react';
// ...
const touchStartRef = useRef(0);
const touchEndRef = useRef(0);
// ...
const handleTouchStart = (e: React.TouchEvent) => {
touchStartRef.current = e.targetTouches[0].clientX;
};
const handleTouchMove = (e: React.TouchEvent) => {
touchEndRef.current = e.targetTouches[0].clientX;
};
const handleTouchEnd = () => {
if (!api) return;
const swipeDistance = touchStartRef.current - touchEndRef.current;
// ...
};
// ...| <Carousel | ||
| setApi={setApi} | ||
| className='w-full max-w-md' | ||
| className='w-full' | ||
| opts={{ | ||
| loop: true, | ||
| loop: false, | ||
| align: 'center', | ||
| }} | ||
| > | ||
| <CarouselContent> | ||
| {slides.map((slide, index) => ( | ||
| <CarouselItem key={index}> | ||
| <div className='flex items-center justify-center overflow-hidden rounded-3xl'> | ||
| <Image | ||
| src={slide.src} | ||
| alt={slide.alt} | ||
| width={slide.width} | ||
| height={slide.height} | ||
| className='object-contain' | ||
| priority={index === 0} | ||
| /> | ||
| <div className='flex flex-col items-center'> | ||
| {/* 텍스트 영역 */} | ||
| <div className='mb-6 flex flex-col items-center text-center'> | ||
| <h2 className='text-text-basic text-[24px] font-[600]'> | ||
| {slide.title} | ||
| </h2> | ||
| <p className='mt-1 text-[16px] font-[500] text-[#746181]'> | ||
| {slide.description} | ||
| </p> | ||
| </div> | ||
|
|
||
| {/* 인디케이터 점들 */} | ||
| <div className='flex gap-2'> | ||
| {slides.map((_, idx) => ( | ||
| <button | ||
| key={idx} | ||
| onClick={() => api?.scrollTo(idx)} | ||
| className={`h-1.5 rounded-full transition-all ${ | ||
| current === idx | ||
| ? 'w-6 bg-gray-700' | ||
| : 'w-1.5 bg-gray-200' | ||
| }`} | ||
| aria-label={`슬라이드 ${idx + 1}로 이동`} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| </CarouselItem> | ||
| ))} | ||
| </CarouselContent> | ||
| </Carousel> |
There was a problem hiding this comment.
현재 캐러셀의 각 아이템(CarouselItem) 내에서 인디케이터 점들을 렌더링하고 있습니다. 이로 인해 slides 배열의 길이만큼 인디케이터 그룹이 중복 생성되어 비효율적이며, 인디케이터가 텍스트와 함께 슬라이드되는 문제가 발생합니다. 인디케이터는 슬라이드되지 않고 고정되어야 하므로, CarouselContent 외부에서 한 번만 렌더링하도록 수정하는 것이 좋습니다.
<Carousel
setApi={setApi}
className='w-full'
opts={{
loop: false,
align: 'center',
}}
>
<CarouselContent>
{slides.map((slide, index) => (
<CarouselItem key={index}>
<div className='flex flex-col items-center'>
{/* 텍스트 영역 */}
<div className='mb-6 flex flex-col items-center text-center'>
<h2 className='text-text-basic text-[24px] font-[600]'>
{slide.title}
</h2>
<p className='mt-1 text-[16px] font-[500] text-[#746181]'>
{slide.description}
</p>
</div>
</div>
</CarouselItem>
))}
</CarouselContent>
<div className='mt-6 flex justify-center gap-2'>
{slides.map((_, idx) => (
<button
key={idx}
onClick={() => api?.scrollTo(idx)}
className={`h-1.5 rounded-full transition-all ${
current === idx
? 'w-6 bg-gray-700'
: 'w-1.5 bg-gray-200'
}`}
aria-label={`슬라이드 ${idx + 1}로 이동`}
/>
))}
</div>
</Carousel>
| if (!api) return; | ||
|
|
||
| const swipeDistance = touchStart - touchEnd; | ||
| const minSwipeDistance = 50; |
There was a problem hiding this comment.
minSwipeDistance 값으로 50이 하드코딩되어 있습니다. 이 값을 컴포넌트 상단에 const MIN_SWIPE_DISTANCE = 50;과 같이 상수로 정의하고 사용하면 코드의 가독성과 유지보수성이 향상됩니다.
1 participant
요약
구현 사항
📸 스크린샷
Need Review
Reference
📜 리뷰 규칙
Reviewer는 아래 P5 Rule을 참고하여 리뷰를 진행합니다.
P5 Rule을 통해 Reviewer는 Reviewee에게 리뷰의 의도를 보다 정확히 전달할 수 있습니다.