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

[홈] 무한 캐러셀 구현 #123

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

[홈] 무한 캐러셀 구현 #123

wants to merge 16 commits into from

Conversation

solbat
Copy link
Member

@solbat solbat commented Dec 15, 2022

제출 전 필수 확인 사항:

  • Merge 하는 브랜치가 올바른가?
  • 코드 컨벤션을 준수하는가?
  • 빌드가 되는 코드인가요?
  • 버그가 발생하지 않는지 충분히 테스트 해봤나요?

작업 내용:

  • 무한 캐러셀 구현

Copy link
Member

@trumanfromkorea trumanfromkorea left a comment

Choose a reason for hiding this comment

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

고생하셨습니당

@@ -15,7 +15,9 @@ final class HomeViewController: UIViewController, BaseViewController {

var viewModel: HomeViewModel?
var disposeBag = DisposeBag()


var datasource: [HomeCapsuleCellItem] = []
Copy link
Member

Choose a reason for hiding this comment

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

  1. datasource 라는 변수명이 CollectionView 의 DataSource 와 헷갈릴 가능성이 있다고 생각합니다!
  2. 해당 데이터는 ViewModel 이 가지고 있는게 자연스럽다고 생각합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. 변경하였습니다!
  2. VM으로 옮겼습니다!

@@ -56,7 +56,7 @@ final class HomeViewModel: BaseViewModel, CapsuleCellNeedable {
owner.output.featuredCapsuleCellItems.accept(
CapsuleType.allCases
.map { owner.getHomeCapsuleCellItem(capsules: capsuleList, type: $0) }
.compactMap({ $0 })
.compactMap({ $0 }) // + Array(repeating: nil, count: Int.max)
Copy link
Member

@trumanfromkorea trumanfromkorea Dec 15, 2022

Choose a reason for hiding this comment

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

큰 상관은 없지만 소괄호를 제외해도 될것 같아요

그리고 59번째 라인을 없애고 58번째 라인의 map 메소드를 compactMap 으로 바꿔도 같은 기능을 하지 않을까 싶네요~

Copy link
Member Author

Choose a reason for hiding this comment

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

compactMap으로 변경하였습니다!

owner.datasource = capsuleCellItems
owner.homeView.capsuleCollectionView.reloadData()
DispatchQueue.main.async {
Thread.sleep(forTimeInterval: 0.01)
Copy link
Member

Choose a reason for hiding this comment

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

여기서 메인스레드를 재우는 이유가 있나요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

앱을 처음 실행 시 간혹 캡슐이 짤려서 두줄로 나오는 버그가 발생하였습니다.
저는 이 문제가 컬렉션뷰가 불린 후 초기 화면 설정을 위해 scrollToItem을 실행 시에 발생하는 버그라고 판단하였습니다.
따라서 먼저 컬렉션뷰가 초기에 멀쩡하게 호출된 후에 텀을 두어 scrollToItem 함수가 실행되도록 하고자 하였습니다........
그러나 간헐적으로 버그가 발생하기는 합니다..

Copy link
Member

@chansooo chansooo left a comment

Choose a reason for hiding this comment

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

LGTM👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants