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

Carousel 컴포넌트 추가 #5

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Carousel 컴포넌트 추가 #5

merged 5 commits into from
Jul 8, 2024

Conversation

dladncks1217
Copy link
Contributor

Why need this PR❓

Carousel 컴포넌트 추가

Screenshoot 🌅 (optional)

스토리북 코드 매우 대충짜긴 했는데 아래와 같이 쓰면 됩니당
스크린샷 2024-07-07 오후 4 53 26

이미지나 컴포넌트 들어가는 부분에 Wrapper로 묶어주시고, Dots나 Move컴포넌트는 사용 시 집어넣으면 됩니다.
Carousel로 Wrapping되어있어야 하긴 하구요.
optional 한 속성도 꽤 있어서 쳐낼거 쳐내면서 사용해주시면 됩니당

@dladncks1217 dladncks1217 self-assigned this Jul 7, 2024
@dladncks1217 dladncks1217 requested a review from moong23 as a code owner July 7, 2024 07:56
Copy link
Contributor

@moong23 moong23 left a comment

Choose a reason for hiding this comment

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

지금 구조에서 viewIndex가 변경될 때 모든 컴포넌트가 리랜더링되는데, viewIndex부분만 분리해서 Provider를 2중으로 제공하면 리랜더링되는 파트를 줄일 수 있을 것 같기도...합니다. 이 부분 오늘 집에 가서 한번 수정해서 커밋해보겠습니다

@moong23 moong23 self-requested a review July 8, 2024 01:46
Copy link
Contributor

@moong23 moong23 left a comment

Choose a reason for hiding this comment

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

Comment에 남긴 것 처럼
viewIndex의 context만 분리해서 이중으로 Provide하는 방법을 통해
리랜더링되는 부분을 조금 줄였습니다.
애초에 Native로 제작되어 매우 가벼워서 그럴필요가 있나 싶긴했지만
그래도 애니메이션 들어가고 이미지 들어가게 되면 랜더링 최소화하면 좋을것같아서
개발 초반에 열정 넘칠때 조금이라도 더 작업해놓았습니다 ㅎㅎ
수정된 부분 확인하신 후 merge해주시면 될 것 같아요~! 👍

@dladncks1217
Copy link
Contributor Author

@moong23
좋습니다 👍👍
캐러셀 움직이는 버튼 연타했을때 움직임이 너무 별로라 일단 쓰로틀링 달아놨어용
근데 이런 훅들은 범용적으로 쓰일법한 친구인데 이런친구들 어디에 위치시키는게 좋을것같나요?

@dladncks1217
Copy link
Contributor Author

구두로합의한대로 수정했고 머지하겠습니당

@dladncks1217 dladncks1217 merged commit 2631941 into dev Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants