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

박스오피스II [STEP 2] yetti, maxhyunm #111

Open
wants to merge 7 commits into
base: ic_9_yetti
Choose a base branch
from

Conversation

iOS-Yetti
Copy link

안녕하세요 델마!(@delmaSong)
STEP1 수정과 STEP2를 함께 작업해서 올려드립니다!
이번에도 리뷰 잘 부탁드립니다🙇‍♀️

고민했던 점

화면모드 변경 버튼 구현

  1. 버튼
    UIButton을 이용해서 구현해보았는데 UIButton을 사용해 구현하게 되면 직접 구현해줘야하는 부분이 많아 코드가 길어졌고 backgroundColor 역시 요구사항에서처럼 뽑아낼 수 없었습니다.
  2. UITabbarController
    NavigationController처럼 하단의 Tabbar를 관리하는 UITabbarController를 활용하는 방법도 고민해 보았습니다.
    다만 이렇게 구현할 경우 NavigationController와 TabbarController가 혼재하는 복잡한 방식으로 구현하게 되고, 불필요하게 컨트롤러 설정을 위한 코드가 추가되기 때문에 바람직한 방법이 아닌 것 같았습니다.

이 내용은 뷰의 toolbarItems 프로퍼티에 아래와 같이 Array를 추가함으로써 간단하게 구현할 수 있었습니다.

let modeChangeButton = UIBarButtonItem(title: "화면 모드 변경", style: .plain, target: self, action: #selector(hitChangeModeButton))
let flexibleSpace = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: self, action: nil)
...
self.toolbarItems = [flexibleSpace, modeChangeButton, flexibleSpace]

flexibleSpace는 툴바에 들어가는 아이템들의 spacing을 일정하게 조절할 수 있도록 사용됩니다. 여기서는 화면 모드 변경 아이템을 중앙에 배치하기 위해 좌우로 flexibleSpace를 추가해 주었습니다.

Grid 형식 아이콘 크기 설정

아이콘 방식으로 화면을 구현하게 되면 한 줄에 두 개의 박스가 들어가도록 레이아웃을 설정해야 합니다. 이때 아이콘의 크기를 어떻게 설정해주며 좋을지 고민이 있었습니다. 저희는 view 자체의 너비 즉 view.frame.width에서 좌/우 및 중앙 여백을 뺀 값을 2로 나눈 값으로 아이콘의 가로와 세로를 설정하였습니다.

셀 재활용으로 인한 문제 해결

Collection View에서 셀을 재사용하면서, 검은 색으로 들어가야 하는 순위 텍스트가 빨간색으로 잘못 들어가는 부분을 확인했습니다.

해당 부분은 PrepareForReuse()를 통해 아래와 같이 폰트 색상을 초기화해 주었습니다.

override func prepareForReuse() {
    rankIntensityLabel.textColor = .black
}

조언을 얻고 싶은 점

셀높이 자동 지정 문제

날짜선택을 통해 다른 데이터를 불러오고 나면 일부 셀의 높이가 초기화되지 못하고 줄어드는 문제를 확인하였습니다.

레이아웃을 초기화시키면 해결될까 싶어서 invalidateLayout()을 넣어보기도 하고, layoutIfNeeded()를 호출해보기도 했는데 해결이 되지 않았습니다.
그래서 아예 셀 관련 레이아웃을 새로 설정할 수 있게 setUpCollectionViewLayout()setUpDataSource()를 다시 호출하니 문제가 해결되었습니다.

현재 정상적으로 앱이 작동하고 있긴 하지만, 이게 제대로된 해결방법인지 의구심이 듭니다 😭
혹시 델마께서는 이런 상황에서 사용할 만한 좋은 해결방법을 알고계실까 싶어서 조언을 얻고 싶습니다!

@delmaSong delmaSong self-requested a review August 18, 2023 01:32
Copy link
Member

@delmaSong delmaSong left a comment

Choose a reason for hiding this comment

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

다이나믹 타입 적용까지!! 넘 수고 많으셨습니다 ㅎㅎ
리뷰 드릴게 크게 없네요, isListMode 변수로 판단하는 부분이 몇 군데 있는데
해당 Bool변수에 의존하는 것 말고 좀 더 역할분리에 신경써서 코드를 개선할 수 있을 것 같아요!
질문 있으시면 편하게 댓글orDM 주세요 :)

Comment on lines +152 to 171
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
}
Copy link
Member

Choose a reason for hiding this comment

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

isListMode 분기를 데이터소스 클로져 안에서 진행하는 편이 코드 가독성 등 더 깔끔할 것 같습니다

Comment on lines +94 to +101
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
Copy link
Member

Choose a reason for hiding this comment

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

지금은 리스트, 아이콘 형태의 UI 뿐인데, 좀 다르게 생긴 UI등이 더 추가된다면 어떨까요?
isListMode라는 불 타입의 변수로 관리하는 것 말고 다른 좋은 방법이 있을 것 같아요.

Comment on lines +18 to +21
guard let urlString = networkType.url, let header = networkType.header else {
completion(.failure(NetworkingError.invalidAPIKey))
return
}
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

Comment on lines +10 to +11
final class BoxOfficeRankingListCell: UICollectionViewListCell {
static let cellIdentifier = "BoxOfficeListCell"
Copy link
Member

Choose a reason for hiding this comment

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

String(describing:) 에 대해 알아보셔요
셀의 identifier는 가능한 클래스명과 동일하게 하는 게 나중에 덜 헷갈리더라고요

Comment on lines 243 to +248
func setUpDate(_ date: Date) {
self.date = date
self.title = getDateString(format: Namespace.dateWithHyphen)

setUpCollectionViewLayout()
setUpDataSource()
Copy link
Member

Choose a reason for hiding this comment

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

질문 남겨주신 사항 중 여기서 두 메서드 다시 호출하는 방식보다 아마 말씀해주신 셀의 UIStackView쪽을 한번 살펴보시는 걸 추천드려요! 스택뷰의 arrangedSubview에 보여줄 게 없으면 크기가 적절히 줄어드는 게 장점이긴 한데, 어느정도 고정된 높이를 가져야하는 경우에는 또 잘 줄어들어버려서 말씀하신 그런 문제가 종종 발생하는 것 같더라고요.

delmaSong

This comment was marked as resolved.

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.

None yet

3 participants