-
Notifications
You must be signed in to change notification settings - Fork 0
Week3 #3
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
base: main
Are you sure you want to change the base?
Week3 #3
Conversation
| private lazy var realTimeLivePopularCollectionView: UICollectionView = { | ||
| let layout = UICollectionViewFlowLayout() | ||
| layout.scrollDirection = .horizontal // scrollDirection의 기본값은 .vertical이다 | ||
|
|
||
| let inset: CGFloat = 16.0 | ||
| layout.itemSize = CGSize(width: (UIScreen.main.bounds.width - inset*2.0)/2.5, height: 146) | ||
| layout.sectionInset = UIEdgeInsets(top: inset, left: inset/2, bottom: inset, right: inset) | ||
| layout.minimumLineSpacing = 4.0 | ||
|
|
||
| let collectionView = UICollectionView(frame: .zero, collectionViewLayout: layout) | ||
| collectionView.backgroundColor = .systemBackground | ||
| collectionView.showsHorizontalScrollIndicator = false | ||
| collectionView.delegate = self | ||
| collectionView.dataSource = self | ||
|
|
||
| collectionView.register(RealTimePopularLiveCell.self, forCellWithReuseIdentifier: RealTimePopularLiveCell.identifier) | ||
|
|
||
| return collectionView | ||
| }() |
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.
이 쪽 코드가 중복돼서 고민인 것 같아요
다른 점을 보니깐 Cell과 itemSize정도인데, 하나의 공통 메소드를 만들어서 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.
저도 이부분이 좀 고민이었는데 팟장님의말씀 너무 좋은것같습니다 👍
| } | ||
|
|
||
| func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell { | ||
| if collectionView == top20CollectionView { |
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.
if문 보다는 switch문을 사용해보는거 어떨까요??
y-eonee
left a comment
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.
코드 잘보았습니다!! 개인적으로 모델파일은 안만들시고 하드코딩으로 하신 것 같아서 조금 아쉽습니다 그리고 폰트랑 컬러 한번 설정하면 좀 더 디테일 있는 뷰가 될 것 같아요 👍 과제하느라 고생많으셨습니다!
|
|
||
| private let noticeButton: UIButton = { | ||
| let button = UIButton() | ||
| button.setTitle("공지 티빙 계정 공유 정책 추가 안내 >", for: .normal) |
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.
title 텍스트에 이렇게 넣게 되면 나중에 화면 크기가 바뀌었을 때 > 가 원하는 위치에 가지 않을 수도 있을 것 같아요! >를 따로 빼서 오토레이아웃으로 정렬해주는건 어떨까요?
| private lazy var realTimeLivePopularCollectionView: UICollectionView = { | ||
| let layout = UICollectionViewFlowLayout() | ||
| layout.scrollDirection = .horizontal // scrollDirection의 기본값은 .vertical이다 | ||
|
|
||
| let inset: CGFloat = 16.0 | ||
| layout.itemSize = CGSize(width: (UIScreen.main.bounds.width - inset*2.0)/2.5, height: 146) | ||
| layout.sectionInset = UIEdgeInsets(top: inset, left: inset/2, bottom: inset, right: inset) | ||
| layout.minimumLineSpacing = 4.0 | ||
|
|
||
| let collectionView = UICollectionView(frame: .zero, collectionViewLayout: layout) | ||
| collectionView.backgroundColor = .systemBackground | ||
| collectionView.showsHorizontalScrollIndicator = false | ||
| collectionView.delegate = self | ||
| collectionView.dataSource = self | ||
|
|
||
| collectionView.register(RealTimePopularLiveCell.self, forCellWithReuseIdentifier: RealTimePopularLiveCell.identifier) | ||
|
|
||
| return collectionView | ||
| }() |
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.
저도 이부분이 좀 고민이었는데 팟장님의말씀 너무 좋은것같습니다 👍
| } | ||
| 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.
if문 조건으로 어떤 걸 구분하려고 하신건지 궁금합니다!!
| class MainCollectionViewCell: UICollectionViewCell { | ||
|
|
||
| static let identifier = "MainCollectionViewCell" | ||
| private let scrollView = UIScrollView() |
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.
저도 처음에 스크롤뷰로 짰었는데요. 재현님처럼 끝까지 스크롤이 잘 안되는 이슈가 있었어요..! 테이블뷰로 한번 수정해보시는 것은 어떨까요?
KHW01104
left a comment
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.
이번 과제도 너무 고생하셨어요!! 😃
코드의 가독성과 간결함을 위해 노력하신 흔적들이 보여서 본받고 깨닫는 시간을 가질 수 있었습니다.
그리고 리뷰 요구사항을 남겨주셨는데요, 저도 처음에 재현님과 똑같이 하나의 VC만으로 구현하다가 자료들의 구조라던가, 코드들이 불필요하게 길어지는게 보여서 각 모듈별로 VC를 나누고 하나의 mainVC로 합쳐서 구현했습니다! 확실히 후자의 방법이 더 좋은거 같더라구요,,, 참고해주시면 감사하겠습니다!!
| let inset: CGFloat = 16.0 | ||
| layout.itemSize = CGSize(width: (UIScreen.main.bounds.width - inset*2.0)/2.5, height: 146) | ||
| layout.sectionInset = UIEdgeInsets(top: inset, left: inset/2, bottom: inset, right: inset) | ||
| layout.minimumLineSpacing = 4.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.
let inset : CGFloat는 어떤 기능이나 구현을 위해 사용하는 걸까요?
| func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell { | ||
| if collectionView == top20CollectionView { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: Top20CollectionViewCell.identifier, for: indexPath) as? Top20CollectionViewCell else {return UICollectionViewCell()} | ||
| cell.configure(rank: indexPath.row + 1, image: UIImage(named: "harryPotter")) | ||
| return cell | ||
| } else if collectionView == realTimeLivePopularCollectionView { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: RealTimePopularLiveCell.identifier, for: indexPath) as? RealTimePopularLiveCell else {return UICollectionViewCell()} | ||
| cell.configure(rank: indexPath.row + 1, image: UIImage(named: "transportLove")) | ||
| return cell | ||
| } else if collectionView == realTimeMovieCollectionView { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: RealTimeMovieCollectionViewCell.identifier, for: indexPath) as? RealTimeMovieCollectionViewCell else {return UICollectionViewCell()} | ||
| cell.configure(rank: indexPath.row + 1, image: UIImage(named: "signal")) | ||
| return cell | ||
| } else if collectionView == baseBallCollectionView { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: BaseballCollectionViewCell.identifier, for: indexPath) as? BaseballCollectionViewCell else {return UICollectionViewCell()} | ||
| cell.configure(rank: indexPath.row + 1, image: UIImage(named: "image 103")) | ||
| cell.backgroundColor = .white | ||
| return cell | ||
| } else if collectionView == TVCollectionView { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: TVCollectionViewCell.identifier, for: indexPath) as? TVCollectionViewCell else {return UICollectionViewCell()} | ||
| cell.configure(rank: indexPath.row + 1, image: UIImage(named: "appleTV")) | ||
| cell.backgroundColor = .black | ||
| return cell | ||
| } else if collectionView == lifeMovieCollectionView { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: lifeMovieCollectionViewCell.identifier, for: indexPath) as? lifeMovieCollectionViewCell else {return UICollectionViewCell()} | ||
| cell.configure(rank: indexPath.row + 1, image: UIImage(named: "image")) | ||
| cell.backgroundColor = .black | ||
| 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.
if~else 보다 코드의 간결화를 위해 다른 방법을 사용해보시면 어떨까요?
그러면 가독성 측면에서 좋을거 같아요!!
| private let posterImage: UIImageView = { | ||
| let imageView = UIImageView() | ||
| imageView.backgroundColor = .black | ||
| imageView.image = UIImage(named: "transportLove") | ||
| imageView.contentMode = .scaleAspectFit | ||
| return imageView | ||
| }() | ||
|
|
||
| private let rankLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.text = "1" | ||
| label.font = .systemFont(ofSize: 30 ,weight: .bold) | ||
| label.textColor = .white | ||
| label.transform = CGAffineTransform(rotationAngle: .pi / 30) | ||
| label.textAlignment = .center | ||
| return label | ||
| }() | ||
|
|
||
| private let titleLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.text = "JTBC" | ||
| label.font = .systemFont(ofSize: 12 ,weight: .regular) | ||
| label.textColor = .white | ||
| label.textAlignment = .center | ||
| return label | ||
| }() | ||
|
|
||
| private let episodeLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.text = "이혼숙려캠프 34화" | ||
| label.font = .systemFont(ofSize: 12 ,weight: .thin) | ||
| label.textColor = .white | ||
| label.textAlignment = .center | ||
| return label | ||
| }() | ||
|
|
||
| private let ratingLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.text = "27.2%" | ||
| label.font = .systemFont(ofSize: 12 ,weight: .bold) | ||
| label.textColor = .white | ||
| label.textAlignment = .center | ||
| return label | ||
| }() | ||
|
|
||
| private lazy var verticalStack: UIStackView = { | ||
| let stack = UIStackView(arrangedSubviews: [titleLabel, episodeLabel,ratingLabel]) | ||
| stack.axis = .vertical | ||
| stack.spacing = 1 | ||
| stack.alignment = .leading | ||
| stack.distribution = .fillEqually | ||
| return stack | ||
| }() | ||
|
|
||
| private lazy var horizontalStack: UIStackView = { | ||
| let stack = UIStackView(arrangedSubviews: [rankLabel, verticalStack]) | ||
| stack.axis = .horizontal | ||
| stack.spacing = 3 | ||
| stack.distribution = .fill | ||
| stack.alignment = .top | ||
| return stack | ||
| }() |
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.
Pr에 올리신 고민들을 Comment로 남겨 두었습니다 한번 고민해보시고
View를 구현하기 직전 어떤식으로 설계를 하면 좋을까에 대한 고민을 많이 해보면 좋을거 같아요
저같은 경우는 아이패드에 피그마상에 나온 디자인을 캡쳐한다음 goodnote로 그려보면서 설계를 잡는 편입니당 그래도 접근자체가 틀렸다 이런말은 아닙니당 다를 뿐이라고 생각합니다
고민에 흔적들이 곳곳에서 보였습니다. 다른 조원들 코리를 다시면서 어떻게 설계했을까?에 대해서 한번 보시지용 😁❤️😘
| private func setupUI() { | ||
| self.view.addSubview(logoStackView) | ||
|
|
||
| [searchButton,iconImageView].forEach { |
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.
p3 : addSubView 하는 코드 까지 다른 함수로 분리하면 더 가독성이 좋을거 같습니다
| func configure(rank: Int, image: UIImage?) { | ||
| posterImage.image = image | ||
| } |
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.
하나의 셀에서 여러 컬렉션뷰 섹션을 보여주는 복합 UI 구조인거 같은데
이게 활용적인 측면에서 좋은지는 잘 모르겠습니다 사실상 Cell이 뷰컨의 느낌을 가지는거 같고
UICollectionViewDelegate, UICollectionViewDelegate 를 Cell이 채택하여 구현하는 경우는 잘 보지 못한거 같아서
만약 해당 구조를 가져갈것이라면 하나의 뷰컨에서 해당 컬렉션뷰와, Cell을 찍어 낼수 있는 클래스를 만들고 사용해볼거 같습니다
그런다면 재사용성과 Cell 자체의 코드 덩어리들이 많이 줄거 같아요 아니면 뷰컨에서 개별적인 컬랙션뷰를 여러개를 가지는 구조
또는 컴포지셔널 레이아웃을 사용하는등의 방법이 있을거 같아요
하지만 시도 자체는 너무 흥미로운거 같습니다! 😁
|
|
||
| return view | ||
| }() | ||
|
|
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.
요런 간격 줄여주는것도 가독성 확보에 좋을거 같아용
| private extension PagingTabBarCell { | ||
| func setupLayout() { | ||
| [ | ||
| titleLabel, | ||
| underline, | ||
|
|
||
| ].forEach { addSubview($0) } | ||
| titleLabel.snp.makeConstraints { | ||
| $0.leading.top.trailing.equalToSuperview() | ||
| } | ||
| underline.snp.makeConstraints { | ||
| $0.height.equalTo(3.0) | ||
| $0.top.equalTo(titleLabel.snp.bottom).offset(4.0) | ||
| $0.leading.trailing.bottom.equalToSuperview() | ||
| } | ||
|
|
||
| } | ||
| } |
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으로 빼셨나용?
|
|
||
|
|
||
| } | ||
| func configure(rank: Int, image: UIImage?) { |
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으로 빼보깅
| label.text = "1" | ||
| label.font = .systemFont(ofSize: 30 ,weight: .bold) | ||
| label.textColor = .white | ||
| label.transform = CGAffineTransform(rotationAngle: .pi / 30) |
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.
요게 어떤 속성자인가용
#️⃣과제 내용
📝작업 내용
스크린샷 (선택)
💬리뷰 요구사항(선택)
paginView 관관련해서 이런식으로 한 화면을 한 collectionView cell로 보여주는게 좋을지 아니면 VC 를 여러개 만들어서 따로 관리하면 좋을지 의논해보면 좋을 것 같아요!!
그리고 collectionView 사용하면서 중복되는 코드가 엄청 많다고 느꼈는데 이것을 어떻게 하면 중복을 줄일수 있을지 의논해보고 싶습니다!!