-
Notifications
You must be signed in to change notification settings - Fork 16
[Feat] #483 - 신규 홈뷰 - DiffableDataSource 적용 #492
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
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.
논의때 나온 내용 잘 반영해주셨네요! 최공공
func getHomeDescription() -> AnyPublisher<HomeDescriptionModel, Never> | ||
func getRecentSchedule() -> AnyPublisher<HomeRecentScheduleModel, Never> | ||
func getAppServices() -> AnyPublisher<[HomeAppServicesModel], Never> | ||
func getInsightPosts() -> AnyPublisher<[HomeInsightPostsModel], Never> | ||
func getGroupPosts() -> AnyPublisher<[HomeGroupPostModel], Never> | ||
func getCoffeeChatPosts() -> AnyPublisher<[HomeCoffeeChatPostModel], Never> |
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.
좋아요~~
var id: UUID { | ||
switch self { | ||
case .homePage, .instagram, .youtube: | ||
return UUID() | ||
} | ||
} | ||
|
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.
타입별로 id 분기처리가 필요하지 않다면 let id = UUID() 로 해도 좋을 것 같아용
extension CoffeeChatCardCVC { | ||
func configureCell(model: HomeCoffeeChatPostModel?) { | ||
func configureCell(model: HomePresentationModel.CoffeeChat?) { | ||
guard let model else { return } | ||
|
||
self.titleLabel.text = model.bio |
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.
옵셔널이 이유는 visitor일땐 nil값이 들어올수 있기 때문인가여?
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.
(모든 셀의) configureCell에서 모델을 다 옵셔널로 받고 있었는데, 해당 부분 수정했습니다!
func hash(into hasher: inout Hasher) { | ||
switch self { | ||
case .description(let model): | ||
hasher.combine(model.id.hashValue) | ||
case .recentSchedule(let model): | ||
hasher.combine(model.id.hashValue) | ||
case .productService(let model): | ||
hasher.combine(model.id.hashValue) | ||
case .appService(let model): | ||
hasher.combine(model.id.hashValue) | ||
case .insightPost(let model): | ||
hasher.combine(model.id.hashValue) |
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.
해싱함수 직접 구현해주셨군용
enum HomeForMemeberItem: Hashable {
case description(HomePresentationModel.Description)
}
struct HomePresentationModel {
struct Description: Identifiable, Hashable { ... }
}
위 구조처럼 HomeForMemeberItem에서 hash 직접 정의하지 않는 방법은 어떤가용
이유는 구현이 간편해서 입니다!
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.
아...하! 이렇게 해주면 되는군요 ...
protocol HomeForMemberItemProvider { | ||
// cells | ||
func createDashBoardCellRegistration() -> UICollectionView.CellRegistration<DashBoardCardCVC, HomePresentationModel.Description> | ||
func createCalendarCellRegistration() -> UICollectionView.CellRegistration<CalendarCardCVC, HomePresentationModel.RecentSchedule> | ||
func createProductCellRegistration() -> UICollectionView.CellRegistration<MainProductCardCVC, HomePresentationModel.ProductService> | ||
func createAppServiceCellRegistration() -> UICollectionView.CellRegistration<AppServiceCardCVC, HomePresentationModel.AppService> |
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.
셀 개수가 많아서 registration 로직을 분리하고 싶은 목적이랑, 이후에 셀이 추가될 때의 확장성을 생각해서 추상화했습니다.
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.
해당 protocol을 여러 vc에서 채택해야한다면 프로토콜로 묶는 이점이 있을 것 같아요. 만약 하나의 vc에서만 채택해야한다면 protocol 생성없이 extension으로만 분리하는 것도 고려해보면 좋을 것 같아요. 다형성이 필요한 로직도 아닌 것 같아서 ..!
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.
움 .. 원래는 HomeForMember랑 HomeForVisitor의 공통 부분을 묶으려고 했었는데, 지금 각각 만들고 있긴 하거등요...
없애도 될 것 같아요!
private func applySnapshot(with appService: [HomePresentationModel.AppService]) { | ||
var snapshot = NSDiffableDataSourceSnapshot<HomeForVisitorSectionLayoutKind, HomeForVisitorItem>() | ||
|
||
snapshot.appendSections(HomeForVisitorSectionLayoutKind.allCases) | ||
|
||
snapshot.appendItems([.description(HomePresentationModel.Description(description: ""))], toSection: .dashBoard) | ||
snapshot.appendItems(self.viewModel.productServiceList.map { .productService($0) }, toSection: .mainProduct) | ||
snapshot.appendItems(appService.map { .appService($0) }, toSection: .appService) | ||
|
||
dataSource.apply(snapshot, animatingDifferences: true) |
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.
전 옛날에 디퍼블 사용할 때 snapshot appendItems하기 전에 deleteItem 했었던 것 같아용
///옛날 코드,,
func loadSnapshot(with assets: [PHAsset]) {
var snapshot = selectedImagesDataSource.snapshot()
let previousAssets = snapshot.itemIdentifiers(inSection: .main)
snapshot.deleteItems(previousAssets)
snapshot.appendItems(assets, toSection: .main)
self.selectedImagesDataSource.apply(snapshot, animatingDifferences: true)
if previousAssets.count < assets.count {
rootView.scrollToRight()
}
}
해당 함수 여러번 호출될 때 데이터 어떻게 쌓이는 지 확인해보면 좋을 것 같아용
+++
해당 함수내에서 새로운 snapshot 인스턴스를 만들어서 중복된 데이터가 쌓이진 않을 것 같네요.
diffable을 사용 시, 새로운 데이터를 반영할 때 snapshot을 새롭게 만들기보단 이전에 사용했던 스냅샷인 datasource.snapshot()을 가져와서 해당 snapshot에서 값을 수정해야 하는 것으로 알고 있어요.
이유는 diffabledatasource는 값이 수정될 때 hashable한 특성을 통해 snapshot의 값 변화 중 변경된 부분만 파악해서 적용하는 걸로 알고 있어요.
이때 snapshot을 매번 새롭게 만들더라도 위의 이점이 적용될지는 ... 더보기
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.
diffable을 사용 시, 새로운 데이터를 반영할 때 snapshot을 새롭게 만들기보단 이전에 사용했던 스냅샷인 datasource.snapshot()을 가져와서 해당 snapshot에서 값을 수정해야 하는 것
맞아요 지금 상태에서는 viewDidLoad 할 때 apply가 한 번 호출되어서 append만 해두었는데, pull to refresh 이후 데이터를 다시 업데이트 할 때는 말씀해주신 것처럼 이전 snapshot 값을 가져와서 업데이트 하는 식으로 구현해야 할 것 같아요. 일단은 새로고침이 필요 없어서 유지해둘게요!
struct HomePresentationModel { | ||
|
||
let description: HomePresentationModel.Description | ||
let recentSchedule: HomePresentationModel.RecentSchedule | ||
let appServices: [HomePresentationModel.AppService] | ||
let insightPosts: [HomePresentationModel.InsightPost] | ||
let groupPosts: [HomePresentationModel.GroupPost] | ||
let coffeeChatPosts: [HomePresentationModel.CoffeeChat] | ||
let announcementPosts: [HomePresentationModel.Announcement] | ||
|
||
// MARK: - Item Structs |
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.
유지보수할 때 직관적으로 잘 파악될 것 같아요 👍
// TODO: 서버 연결 필요 | ||
let announcementInfoList: [AnnouncementInfo] = [ | ||
AnnouncementInfo(id: 0, categoryName: "취업/진로", categoryDetailName: "꿀팁", title: "이력서를 쓰며 확인해야 할 7가지", profileImage: "https://i.pinimg.com/736x/5e/83/3a/5e833a2a1b804d3f0fdd0bae204c20f8.jpg", name: "눈멍이", content: "사진이 있는 경우, 본문 내용은 한 줄로 보여주는 정책으로", images: "https://i.pinimg.com/736x/c5/e4/b7/c5e4b76374f640585796b8e01f907845.jpg"), | ||
AnnouncementInfo(id: 0, categoryName: "취업/진로", categoryDetailName: "꿀팁", title: "이력서를 쓰며 확인해야 할 7가지", profileImage: "https://i.pinimg.com/736x/5e/83/3a/5e833a2a1b804d3f0fdd0bae204c20f8.jpg", name: "눈멍이", content: "https://toss.im/career/job-detail?job_id=4071133003&company=토스뱅크 안녕하세요 31기 안드로이드 파트장 이현우입니다. 제가 속한 토스커뮤니티 계열사중 토스뱅크에서 네이티브 모바일 개발자를 채용을 시작했습니다. 토스커뮤니티에 관심있으신 분은 저에게 디스코드/링크드인 DM 주시면 감사하겠습니다 https://toss.im/career/job-detail?job_id=4071133003&company=토스뱅크 안녕하세요 31기 안드로이드 파트장 이현우입니다. 제가 속한 토스커뮤니티 계열사중 토스뱅크에서 네이티브 모바일 개발자를 채용을 시작했습니다. 토스커뮤니티에 관심있으신 분은 저에게 디스코드/링크드인 DM 주시면 감사하겠습니다", images: nil), |
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.
현우형 잘가
public class DefaultHomeUseCase { | ||
|
||
private let repository: HomeRepositoryInterface | ||
private let cancelBag = CancelBag() | ||
|
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.
이제 useCase에서 스트림 관리하지 않기에 cancelBag() 없어져도 될 것 같네용
@meltsplit 빠른 리뷰 남겨주셔서 감사합니다 🙇🏼 |
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.
@dlwogus0128
리뷰 반영해주셔서 감사합니다 :)
반영해주신 내용 중에 수정할 부분 있어서 코리코리 남겨요.
case .homePage, .instagram, .youtube: | ||
return UUID() | ||
} | ||
return UUID() |
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.
연산프로퍼티로하면 id값에 접근할 때 마다 새로운 uuid가 반환돼서 고유성을 잃을 것 같아요.
아래와 같이 변경해주세요!!
let id = UUID()
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.
SocialLinkCardType이 enum이라서 저장 프로퍼티가 불가능해갖구요
말씀해주신 대로 매번 UUID() 생성하면 안될 것 같고, 아래처럼 description 값으로 id 지정하는 게 더 좋을 것 같습니다.
다음 이슈에 반영해둘게요!
var id: String {
return self.description
}
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.
근데 생각해보니까 지금 PresentationModel에서도 각 Model마다 매번 UUID를 새로 생성하고 있는데,
이러면 매번 아이디 값이 부여되니까 디퍼블 사용하는 의미가 없겠네요 ㅋㅋ
해당 부분도 고유값으로 구분할 수 있게 수정해두겠습니다 감사해요!
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.
SocialLinkCardType이 enum이라서 저장 프로퍼티가 불가능해갖구요
enum은 저장프로퍼티가 안됐었군요 하하..!
제안 주신 방법은 서로 다른 case가 같은 description을 가지면 동일한 id로 판별 될 수 있는 점이 우려스럽네용. 구글링해서 더 좋은 레퍼런스 가져와봤습니다
// associated Value 없을 때
enum Foo: Identifiable {
var id: Self {
self
}
case hi
}
// associated Value 있을 때
enum POO: Identifiable, Hashable {
var id: Self {
self
}
case hi(String)
}
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.
제안 주신 방법은 서로 다른 case가 같은 description을 가지면 동일한 id로 판별 될 수 있는 점이 우려스럽네용.
동의합니다! 지금 description이 모두 달라서 이렇게 지정했는데, 우려하시는 점에 동감합니다
근데 enum의 각 case가 Hashable을 준수해서, id를 self로 해도 동작하는군요 ...! 새롭게 배워갑니다!! id값 self로 지정해둘게요 🙇🏼
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.
.
case .homePage, .instagram, .youtube: | ||
return UUID() | ||
} | ||
return UUID() |
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.
SocialLinkCardType이 enum이라서 저장 프로퍼티가 불가능해갖구요
enum은 저장프로퍼티가 안됐었군요 하하..!
제안 주신 방법은 서로 다른 case가 같은 description을 가지면 동일한 id로 판별 될 수 있는 점이 우려스럽네용. 구글링해서 더 좋은 레퍼런스 가져와봤습니다
// associated Value 없을 때
enum Foo: Identifiable {
var id: Self {
self
}
case hi
}
// associated Value 있을 때
enum POO: Identifiable, Hashable {
var id: Self {
self
}
case hi(String)
}
case .homePage, .instagram, .youtube: | ||
return UUID() | ||
} | ||
return UUID() |
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.
작업 너무 고생하셨습니다 !! 🫶
석우님이 코드리뷰를 너무 잘 남겨주셔서 typealias
얘기만 주구장창 ! 해버렸네요ㅎㅎ
PR에 남겨주신 내용과 이전에 남겨주신 코드리뷰 내용에 의견 몇 자 덧붙여봅니다.
우선 석우님과 재현님이 해당 리뷰에서 논의해주신 것처럼 저도 현재 구조는 diffable datasource
의 장점이 적용된 것 같진 않아요.
재현님께서도 리프레시가 없기 때문에 모든 섹션을 업데이트 해주셨다고 했는데, 만약 리프레시 기능이 도입될 경우 섹션별 업데이트까지 추가하면 디퍼블의 이점이 더 극대화될 듯 합니다!
HomeUseCase에서 각 섹션에 대한 API가 분리되어 있으니 ViewModel에서 zip으로 묶지 않고 섹션별로 스트림을 보내준 다음, VC에서 섹션별로 snapshot을 업데이트 해준다면 디퍼블의 장점을 살릴 수 있지 않을까요?! (상상 코드 . .)
홈뷰에 필요한 모든 데이터를 HomePresentationModel로 묶어 View에 전달하기 위해, HomePresentationModel의 상수로 각 item의 struct를 지정했습니다. (더 좋은 방식이 있을까요?)
그렇게 되면 DTO도 HomePresentationModel
로 묶지 않고 각각의 struct로 분리하면 좋겠네요!
저도 Soptlog
에서 하나의 struct 안에 여러개의 struct를 넣어 관리했는데, soptlog는 하나의 API로 모든 데이터를 받아오기 때문에 VC에서 사용이 편리하도록 분리한 것이었습니다!
하지만 홈뷰와 같이 여러개의 API로 각 섹션의 정보를 받아올 수 있는 경우라면 분리하는 것도 좋다고 생각해요~!
@@ -83,6 +83,7 @@ public extension UILabel { | |||
/// - defaultFont, defaultColor에는 기본 폰트와 컬러를 넣어주세요 | |||
func htmlToString(targetString: String, | |||
defaultFont: UIFont, | |||
boldFont: UIFont, |
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.
추후 다른 곳에서 이 함수를 쓸 때 boldFont가 필수값이 아닌 경우도 있을 것 같아요. 옵셔널로 두는건 어떠신가요?
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.
넹! 바꿔두겠습니다
@@ -140,7 +140,7 @@ extension AnnouncementCardCVC { | |||
// MARK: - Methods | |||
|
|||
extension AnnouncementCardCVC { | |||
func configureCell(model: HomeAnnouncementModel?) { | |||
func configureCell(model: HomePresentationModel.Announcement?) { | |||
guard let model else { return } |
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.
석우님이 남겨주신 부분인데 누락된 듯 합니다!
func createDashBoardCellRegistration() -> UICollectionView.CellRegistration<DashBoardCardCVC, HomePresentationModel.Description> { | ||
collectionView.createCellRegistration { [weak self] cell, _, item in | ||
guard let self else { return } | ||
cell.configureCell(userType: self.viewModel.userType, description: item.description) | ||
} | ||
} |
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.
혹시 typealias
를 사용하는 건 어떠신가요? 저는 제네릭 타입이 긴 경우 typealias를 사용해서 가독성을 높여주는 편입니다!
해당 코드에서는 그럴 일이 없을 것 같지만 타입이 변경되었을 때도 typealias 부분만 수정하면 되어서 확장성도 좋다고 생각해요.
사용한다면 아래와 같이 수정할 수 있을 것 같아요.
typealias DashBoardCellRegistration<Cell: UICollectionViewCell> = UICollectionView.CellRegistration<Cell, HomePresentationModel.Description>
func createDashBoardCellRegistration<Cell: DashBoardCardCVC>() -> DashBoardCellRegistration<Cell>
반영하게 된다면 UICollectionView.SupplementaryRegistration
는 사용횟수가 많지 않아 UICollectionView.CellRegistration
만 typealias 사용해도 무방할 듯 합니다~!
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.
아하! 훨씬 가독성이 높아지는 것 같아요 적용해두겠습니다!
extension HomeForVisitorVC: UICollectionViewDataSource { | ||
func numberOfSections(in collectionView: UICollectionView) -> Int { | ||
return HomeForVisitorSectionLayoutKind.allCases.count | ||
dataSource = UICollectionViewDiffableDataSource<HomeForVisitorSectionLayoutKind, HomeForVisitorItem> ( |
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.
여기도 28번째 줄이랑 같이 typealias
를 사용하는건 어떠신가요? (적용하게 된다면 HomeForMemberVC도 같이요!)
@yungu0010 님 말씀대로 새로고침 시에는 섹션별로 snapshot을 업데이트하면 될 것 같아요!
그런데 처음 뷰를 로드했을 때는 초기값을 append 해줘야 하는데, 이 때 섹션별로 로드하면 데이터가 도착하는 속도 차이 때문에 화면에 셀이 랜덤하게 나타나면서 어색해 보일 것 같아서요. 두 가지 방법을 생각해봤는데
보통 이런 경우에 어떻게 처리되나요?? |
🌴 PR 요약
신규 홈뷰(회원/비회원 전용)에 DiffableDataSource를 적용했습니다.
🌱 작업한 브랜치
🌱 PR Point
🍥 HomePresentationModel 생성
HomePresentationModel
로 묶어 View에 전달하기 위해,HomePresentationModel
의 상수로 각 item의struct
를 지정했습니다. (더 좋은 방식이 있을까요?)DiffableDataSource
를 적용하기 위해선 각 모델(아이템)들이Identifiable
해야 해서,HomePresentationModel
내의 struct에서 각 모델들이Identifiable
,Hashable
를 따르도록 구현했습니다.TO-BE
🍥 UseCase: 반환 타입 AnyPublisher으로 변경
PassthroughSubject
변수 대신AnyPublisher
반환 방식으로 수정했습니다.🍥 DiffableDataSoruce 적용
dataSource
에 지정하기 위해서, 각 아이템을Hashable
한 enum으로 래핑하여, 각 케이스마다 모델을 받도록 구현했습니다. (참고)🍥 Item Provider
UICollectionView
의 익스텐션으로 cell과 supplementary view에 대한 Register 함수를 구현해 사용했습니다. (이건 약간 과한 것 같기도 하고요...)🍥 VM -> VC 데이터 전달
🍥 applySnapshot
🍥 HomeSectionKindProtocol
SectionKind
가 활동인지, 비회원 전용인지에 따라 분기 처리가 필요한 곳들이 있었습니다. 기존에는 각각 함수를 만들어뒀었는데, 활동/비회원 SectionKind가 공통으로 따르는 protocol인HomeSectionKindProtocol
를 만들었습니다.🍥 ETC
setUI
,setData
와 같이 적절하지 않은 함수명들을configure
로 통일했습니다.📌 참고 사항
제가 콜렉션뷰랑 친하지 않은 상태라 시행착오를 겪느라 작업이 늦어졌습니다 🥹
(PR이 똥똥해진 것과... 뭉텅이의 커밋들을 용서해주세요)
저보다 콜렉션뷰랑 짱친이신 선배림들의 (ㅎㅎ) 코드리뷰... 기대하겠습니다.
수정해야 할 부분 있으면 거침없는 피드백 부탁드립니다 🙇🏼
(로딩은 다음 이슈에서.. 넣을게요)
📸 스크린샷
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-02-13.at.13.12.13.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-02-12.at.18.24.52.mp4
📮 관련 이슈