-
Notifications
You must be signed in to change notification settings - Fork 1
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
[TASK-61] feat: 캐로젤 공통 컴포넌트 구현 #18
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.
궁금한 사항 2가지만 질문 드렸습니다! approve 해두겠습니다 :)
interface DefaultCarouselPropsType<T> { | ||
slides: T[]; | ||
renderSlide: (card: T) => ReactNode; | ||
spaceSize?: 'small' | 'medium' | 'large'; |
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.
아이콘 사이즈처럼 s, m, l 이렇게 통일하는 건 어떨까요?
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.
네! 좋습니다 수정하겠습니다_!
spaceSize?: 'small' | 'medium' | 'large'; | ||
} | ||
|
||
const DefaultCarousel = <T,>({ |
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.
<T,>
에서 ,는 뭔가요???
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.
이부분은 다른 코드 참고하면서 작성해서 들어간 것 같습니다..!
조금 알아보니 큰 의미는 없고, JSX 문법과 제네릭이 유사항 형태를 가지니 두 가지를 구분하기 위해서 타입 하나만 받는 경우에 ,를 임의로 추가하기도 한다고 합니다..! 이부분은 수정하겠습니다!
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.
억... 일단 그럼 ,로 둘까요?..
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.
generic in tsx로 검색했을 때 먼저 나오는 블로그입니다. tsx에서 제네릭을 쓸 때 대안으로 여러 방식을 사용할 수 있긴한데 현재 방식과 크게 다르지 않은 것 같네요..!
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.
아 그리고 PR 제목에 label이 누락돼 있어서 지난 PR까지 확인하셔서 label 추가해주시면 감사하겠습니다 :)
📝 작업 내용
📸 스크린샷