-
Notifications
You must be signed in to change notification settings - Fork 33
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
박스오피스II [STEP 2] yetti, maxhyunm #111
base: ic_9_yetti
Are you sure you want to change the base?
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.
다이나믹 타입 적용까지!! 넘 수고 많으셨습니다 ㅎㅎ
리뷰 드릴게 크게 없네요, isListMode
변수로 판단하는 부분이 몇 군데 있는데
해당 Bool변수에 의존하는 것 말고 좀 더 역할분리에 신경써서 코드를 개선할 수 있을 것 같아요!
질문 있으시면 편하게 댓글orDM 주세요 :)
if isListMode { | ||
dataSource = UICollectionViewDiffableDataSource<NetworkConfiguration, BoxOfficeEntity.BoxOfficeResult.DailyBoxOffice>(collectionView: self.collectionView) { (collectionView, indexPath, data) -> UICollectionViewCell? in | ||
guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: BoxOfficeRankingListCell.cellIdentifier, for: indexPath) as? BoxOfficeRankingListCell else { | ||
return BoxOfficeRankingListCell() | ||
} | ||
|
||
cell.setUpLabelText(data) | ||
|
||
return cell | ||
} | ||
} else { | ||
dataSource = UICollectionViewDiffableDataSource<NetworkConfiguration, BoxOfficeEntity.BoxOfficeResult.DailyBoxOffice>(collectionView: self.collectionView) { (collectionView, indexPath, data) -> UICollectionViewCell? in | ||
guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: BoxOfficeRankingIconCell.cellIdentifier, for: indexPath) as? BoxOfficeRankingIconCell else { | ||
return BoxOfficeRankingIconCell() | ||
} | ||
|
||
cell.setUpLabelText(data) | ||
|
||
return cell | ||
} |
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.
isListMode 분기를 데이터소스 클로져 안에서 진행하는 편이 코드 가독성 등 더 깔끔할 것 같습니다
if isListMode { | ||
let configuration = UICollectionLayoutListConfiguration(appearance: .plain) | ||
let layout = UICollectionViewCompositionalLayout.list(using: configuration) | ||
|
||
collectionView.collectionViewLayout = layout | ||
} else { | ||
let layout = UICollectionViewFlowLayout() | ||
let width = (view.frame.width - 45) / 2.0 |
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.
지금은 리스트, 아이콘 형태의 UI 뿐인데, 좀 다르게 생긴 UI등이 더 추가된다면 어떨까요?
isListMode
라는 불 타입의 변수로 관리하는 것 말고 다른 좋은 방법이 있을 것 같아요.
guard let urlString = networkType.url, let header = networkType.header else { | ||
completion(.failure(NetworkingError.invalidAPIKey)) | ||
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.
👍🏼
final class BoxOfficeRankingListCell: UICollectionViewListCell { | ||
static let cellIdentifier = "BoxOfficeListCell" |
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.
String(describing:)
에 대해 알아보셔요
셀의 identifier는 가능한 클래스명과 동일하게 하는 게 나중에 덜 헷갈리더라고요
func setUpDate(_ date: Date) { | ||
self.date = date | ||
self.title = getDateString(format: Namespace.dateWithHyphen) | ||
|
||
setUpCollectionViewLayout() | ||
setUpDataSource() |
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.
질문 남겨주신 사항 중 여기서 두 메서드 다시 호출하는 방식보다 아마 말씀해주신 셀의 UIStackView
쪽을 한번 살펴보시는 걸 추천드려요! 스택뷰의 arrangedSubview
에 보여줄 게 없으면 크기가 적절히 줄어드는 게 장점이긴 한데, 어느정도 고정된 높이를 가져야하는 경우에는 또 잘 줄어들어버려서 말씀하신 그런 문제가 종종 발생하는 것 같더라고요.
안녕하세요 델마!(@delmaSong)
STEP1 수정과 STEP2를 함께 작업해서 올려드립니다!
이번에도 리뷰 잘 부탁드립니다🙇♀️
고민했던 점
화면모드 변경 버튼 구현
UIButton
을 이용해서 구현해보았는데UIButton
을 사용해 구현하게 되면 직접 구현해줘야하는 부분이 많아 코드가 길어졌고backgroundColor
역시 요구사항에서처럼 뽑아낼 수 없었습니다.NavigationController
처럼 하단의Tabbar
를 관리하는UITabbarController
를 활용하는 방법도 고민해 보았습니다.다만 이렇게 구현할 경우 NavigationController와 TabbarController가 혼재하는 복잡한 방식으로 구현하게 되고, 불필요하게 컨트롤러 설정을 위한 코드가 추가되기 때문에 바람직한 방법이 아닌 것 같았습니다.
이 내용은 뷰의
toolbarItems
프로퍼티에 아래와 같이 Array를 추가함으로써 간단하게 구현할 수 있었습니다.flexibleSpace
는 툴바에 들어가는 아이템들의spacing
을 일정하게 조절할 수 있도록 사용됩니다. 여기서는 화면 모드 변경 아이템을 중앙에 배치하기 위해 좌우로flexibleSpace
를 추가해 주었습니다.Grid 형식 아이콘 크기 설정
아이콘 방식으로 화면을 구현하게 되면 한 줄에 두 개의 박스가 들어가도록 레이아웃을 설정해야 합니다. 이때 아이콘의 크기를 어떻게 설정해주며 좋을지 고민이 있었습니다. 저희는
view
자체의 너비 즉view.frame.width
에서 좌/우 및 중앙 여백을 뺀 값을 2로 나눈 값으로 아이콘의 가로와 세로를 설정하였습니다.셀 재활용으로 인한 문제 해결
Collection View
에서 셀을 재사용하면서, 검은 색으로 들어가야 하는 순위 텍스트가 빨간색으로 잘못 들어가는 부분을 확인했습니다.해당 부분은
PrepareForReuse()
를 통해 아래와 같이 폰트 색상을 초기화해 주었습니다.조언을 얻고 싶은 점
셀높이 자동 지정 문제
날짜선택을 통해 다른 데이터를 불러오고 나면 일부 셀의 높이가 초기화되지 못하고 줄어드는 문제를 확인하였습니다.
레이아웃을 초기화시키면 해결될까 싶어서
invalidateLayout()
을 넣어보기도 하고,layoutIfNeeded()
를 호출해보기도 했는데 해결이 되지 않았습니다.그래서 아예 셀 관련 레이아웃을 새로 설정할 수 있게
setUpCollectionViewLayout()
과setUpDataSource()
를 다시 호출하니 문제가 해결되었습니다.현재 정상적으로 앱이 작동하고 있긴 하지만, 이게 제대로된 해결방법인지 의구심이 듭니다 😭
혹시 델마께서는 이런 상황에서 사용할 만한 좋은 해결방법을 알고계실까 싶어서 조언을 얻고 싶습니다!