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

DiffableDataSource + Compositional Layout으로 마이그레이션 #58

Merged
merged 14 commits into from
Jan 31, 2024

Conversation

Minny27
Copy link
Member

@Minny27 Minny27 commented Jan 26, 2024

Motivation ⍰

  • 기존에 UICollectionViewDataSource, UICollectionViewDelegate로 collectionview를 관리하고 있었는데
  • 더 효율적으로 관리하고 vc의 부담을 줄여주기 위함

Key Changes 🔑

  • 기존 DataSource, Delegate 코드 삭제
  • DiffableDataSoure, compositional laytout section 생성 및 설정 코드 추가
  • image view content mode 변경
  • sample data image url 추가 및 적용
  • cornerRadius 설정

Compositional layout을 Chat에서도 사용하고 있는데, 하나로 합칠까 하다가, custom하는 방식이 달라서 못했습니다.
이 부분도 나중에 어떻게 하면 좋을 지 고민해봐야 할 거 같습니다.


To Reviewers 🙏🏻

  • 기존이랑 동작 동일한 지 확인해주세요.

Linked Issue 🔗

- 비율이 다른 이미지 링크로 변경
- UICollectionViewCompositionalLayout 확장
- 제약조건 변경
- 필요없는 코드 삭제
- 제약 조건 부분 변경
- 원래 타이머 뷰 텍스트, stroke, progress bar 색상이 동일하게 변화했지만 일부 변경되어 따로 변수 생성
- 테스트 이미지에서 바인딩된 이미지 kf로 할당
- data source 정의 및 설정 코드 추가
- 전달받은 데이터 differable datasource로 바인딩
@Minny27 Minny27 requested review from ibcylon and ChaNoo97 January 26, 2024 15:21
@Minny27 Minny27 self-assigned this Jan 26, 2024
@Minny27 Minny27 added the 🐧 Refactor Code Refactoring label Jan 27, 2024
1. Stream 전달 delegate에서 Observer로 변경
2. VC 로직 VM으로 이동
3. Diffable 관련 type typealias로 관리, 추후 Generic, human error 방지
4. CellViewModel Cell에 주입하는 방식으로 변경
Copy link
Member

@ibcylon ibcylon left a comment

Choose a reason for hiding this comment

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

Compositional로 변경하느라 수고 많으셨습니다.

// self?.imageView.sizeToFit()
// self?.imageView.layoutIfNeeded()
// }
self.imageView.kf.setImage(with: url)
}
Copy link
Member

Choose a reason for hiding this comment

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

sampleData image는 url보니까 token이 있던데 토큰 만료될 일이 없나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이거 픽사베이에서 무료로 이미지 넣어 놓은 거긴한데 만료되는지는 잘 모르겠네요

@@ -13,7 +13,7 @@ import DSKit
final class ProfileCollectionViewCell: UICollectionViewCell {
private lazy var imageView: UIImageView = {
let imageView = UIImageView()
imageView.contentMode = .scaleAspectFit
imageView.contentMode = .scaleToFill
Copy link
Member

Choose a reason for hiding this comment

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

피그마 기준 샘플 이미지에서 여자가 앉아있는 이미지 기준으로 contentMode가 aspectFit이어야 하지 않나요?
또 scaleToFill이 맞다고 해도 scaleToAspectFill이 더 적절할 것 같아요!

navigationItem.leftBarButtonItem = mindImageItem
navigationItem.rightBarButtonItem = notificationButtonItem
}

override func bindViewModel() {
let timeOverSubject = PublishSubject<Void>()
Copy link
Member

Choose a reason for hiding this comment

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

  1. subject를 추가
  2. Cell에 observer로 전달
  3. Cell에서 timeOver signal 전송
  4. VC에서 subject Stream 구독해서 VC's VM으로 전달
  5. VC's VM에서 safe index 계산 후, output 전송
  6. VC에서 View에서 ScrollEvent 수행

Copy link
Member Author

Choose a reason for hiding this comment

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

bindViewModel에서 timeOver 트리거를 PublishSubject로 만들어서 cell에서 timeZero와 바인딩하는 방식 좋은 거 같습니다!

cell.bind(
FallinguserCollectionViewCellModel(userDomain: item),
timerActiveTrigger,
scrollToNextObserver: timeOverSubject
)
Copy link
Member

Choose a reason for hiding this comment

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

  • Cell ViewModel 만들어서 주입
  • timerActive 트리거 주입 및 combineLatest로 변경

let nextCardIndex = userCardScrollIndex
.filter { $0 != 0 }
.map { IndexPath(row: $0, section: 0) }

Copy link
Member

Choose a reason for hiding this comment

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

VC indexPath 로직 VM으로 이동 및 명시적으로 output 분리 ( SRP, 유지보수

let isDimViewHidden = Driver.merge(
timerActiveTrigger.take(1).asDriverOnErrorJustEmpty(), // take(1)을 해주지 않으면 일시정지 때마다 dimmed 됨
timeZero.map { _ in false }
)
Copy link
Member

Choose a reason for hiding this comment

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

DimView hidden state 관리 output
take(1)한 이유는 첫번째 timerTrigger만 capture하기 위해서, 지우면 일시정지할 때 마다 dimView state 변경됨

Copy link
Member Author

Choose a reason for hiding this comment

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

이런 방법이 있는지 몰랐네요!

@@ -13,7 +13,7 @@ import DSKit
final class ProfileCollectionViewCell: UICollectionViewCell {
private lazy var imageView: UIImageView = {
let imageView = UIImageView()
imageView.contentMode = .scaleToFill
imageView.contentMode = .scaleAspectFill
imageView.layer.masksToBounds = true
Copy link
Member

Choose a reason for hiding this comment

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

그냥 scale보다는 AspectScale이 비율 유지할 수 있을 것 같아서 변경
aspectFit에서 fill로 변경한 이유 알려주시면 감사하겠습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

scaleToFill 사용한 이유는 사실 이미지가 꽉 채워졌을 때를 확인하려고 썼던 거긴 해요. 고정으로 할 생각은 없었습니다.
scaleAspectFill을 하게 되면 비율 그대로 확대하다보니, 이미지가 짤립니다.
추후에는 이미지를 비율대로 자르고 확대하는게 어떨까 싶네요
생각해보니까 자르고 확대하나 비율에 맞게 스케일을 키우는 게 똑같은거 같긴하네요

@@ -13,7 +13,7 @@ import DSKit
final class ProfileCollectionViewCell: UICollectionViewCell {
private lazy var imageView: UIImageView = {
let imageView = UIImageView()
imageView.contentMode = .scaleToFill
imageView.contentMode = .scaleAspectFill
imageView.layer.masksToBounds = true
Copy link
Member Author

Choose a reason for hiding this comment

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

scaleToFill 사용한 이유는 사실 이미지가 꽉 채워졌을 때를 확인하려고 썼던 거긴 해요. 고정으로 할 생각은 없었습니다.
scaleAspectFill을 하게 되면 비율 그대로 확대하다보니, 이미지가 짤립니다.
추후에는 이미지를 비율대로 자르고 확대하는게 어떨까 싶네요
생각해보니까 자르고 확대하나 비율에 맞게 스케일을 키우는 게 똑같은거 같긴하네요

typealias ModelType = FallingUser
typealias SectionType = FallingProfileSection
typealias DataSource = UICollectionViewDiffableDataSource<SectionType, ModelType>
typealias Snapshot = NSDiffableDataSourceSnapshot<SectionType, ModelType>
Copy link
Member Author

Choose a reason for hiding this comment

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

typealias로 정의해 놓는게 깔끔하네요

@@ -36,16 +36,18 @@ final class FallingHomeViewController: TFBaseViewController {
let mindImageView = UIImageView(image: DSKitAsset.Image.Icons.mind.image)
let mindImageItem = UIBarButtonItem(customView: mindImageView)

let notificationButtonItem = UIBarButtonItem(image: DSKitAsset.Image.Icons.bell.image, style: .plain, target: nil, action: nil)

let notificationButtonItem = UIBarButtonItem.noti
Copy link
Member Author

Choose a reason for hiding this comment

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

UIBarButtonItem 정의되어 있는게 있는지 몰랐네요


let userCardScrollIndex = Driver.merge(userListObservable, nextScrollIndex).withLatestFrom(userCount) { _, count in
let currentIndex = currentIndexRelay.value
return currentIndex >= count ? count - 1 : currentIndex
Copy link
Member Author

Choose a reason for hiding this comment

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

index가 list count 보다 크거나 같을 때의 조건은 scroll될 때만 예외처리를 해놓은 것입니다.
index가 count -1로 계속 갱신하면 마지막 유저의 타이머가 계속 돕니다.
index의 값은 계속 +1을 하되, count와 같아지는 순간에만 예외적으로 return하는 조건이라 기존 처리를 유지해야 할 거 같습니다.

navigationItem.leftBarButtonItem = mindImageItem
navigationItem.rightBarButtonItem = notificationButtonItem
}

override func bindViewModel() {
let timeOverSubject = PublishSubject<Void>()
Copy link
Member Author

Choose a reason for hiding this comment

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

bindViewModel에서 timeOver 트리거를 PublishSubject로 만들어서 cell에서 timeZero와 바인딩하는 방식 좋은 거 같습니다!

let isDimViewHidden = Driver.merge(
timerActiveTrigger.take(1).asDriverOnErrorJustEmpty(), // take(1)을 해주지 않으면 일시정지 때마다 dimmed 됨
timeZero.map { _ in false }
)
Copy link
Member Author

Choose a reason for hiding this comment

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

이런 방법이 있는지 몰랐네요!

- index가 list count를 넘어갈 때 list count - 1로 설정하면 마지막 유저의 타이머가 무한으로 계속 돌아가서 수정
- 가독성을 위해서 파라미터 이름 만듦
- 스크롤 index가 넘어갈 때의 처리를 위해서 listCount 생성 및 적용
- TFBaseCollectionViewCell를 상속받은UserCardViewCollectionViewCell 구현
- DimView Manager 클래스를 따로 빼려고 했지만, 셀이 생성됐을 때의 x, y좌표를 알아야 해서 cell을 커스텀하는 방식으로 구현함.
- 또한, 셀 각각의 view model의 output을 구독하다보니 다른 셀에 대한 트리거를 처리하기 애매해서 미리 셀에 dimView를 추가해놓고
- 셀에서 시작 시간을 output으로 구독해서 타이머가 시작됐을 때 dim view를 hidden하는 방식으로 구현.
- 특정 셀이 다른 모든 셀의 트리거를 알게 할 수는 있지만, 사이드 이펙트 및 비효율적이어서 이렇게 하지 않음.
@Minny27
Copy link
Member Author

Minny27 commented Jan 31, 2024

추가적으로 DimView가 셀의 프레임에 맞게 show, hidden 동작을 처리했습니다.
시도한 방법은 세 가지 입니다.

  1. DimView Manager 클래스로 dim view 처리 따로 빼기
  • 셀이 생성됐을 때, 셀의 x, y좌표를 알아야 해서 포기
  1. TFBaseCollectionViewCell userCardDimView에 추가
  • 셀의 크기를 알아야하다보니 상태값을 변경해야하는데, base 클래스에서 상태 값을 변경하는 것은 적절하지 않은 거 같았음.
  1. TFBaseCollectionViewCell를 상속받은UserCardViewCollectionViewCell 구현
  • userDimView를 정의해서 해당 셀에서만 사용할 수 있도록 구현

또한, 셀 각각의 view model의 output을 구독하다보니 다른 셀에 대한 트리거를 처리하기 애매해서 미리 셀에 dimView를 추가해놓고 셀에서 시작 시간을 output으로 구독해서 타이머가 시작됐을 때 dim view를 hidden하는 방식으로 구현.

특정 셀이 다른 모든 셀의 트리거를 구독하게 할 수는 있지만, 사이드 이펙트 및 비효율적이라고 생각되서 구현하지 않음.

@Minny27 Minny27 merged commit 029ccb8 into main Jan 31, 2024
@Minny27 Minny27 deleted the Refactor/falling_compositional_layout_apply branch January 31, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐧 Refactor Code Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants