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

[FEAT] iOS 3차 과제 #13

Merged
merged 8 commits into from
Dec 24, 2021
Merged

[FEAT] iOS 3차 과제 #13

merged 8 commits into from
Dec 24, 2021

Conversation

ningpop
Copy link
Member

@ningpop ningpop commented Oct 29, 2021

📌 관련 이슈

#12 [FEAT] iOS 3차 과제

📌 변경 사항 및 이유

  • Xib로 TableView Cell 만들고 AutoLayout 적용
  • CollectionView Cell 만들고 AutoLayout 적용

📌 PR Point

세미나때는 같은 모델을 가지고 TableView와 CollectionView를 만드느라 괜찮았지만,
이번엔 다른 모델을 가지고 활용해야해서 CollectionView의 이미지 불러오는 부분을 완성하지 못했습니다.

성공했오요

📌 참고 사항

다음주는 더 미리미리 해두겠습니다...

📌 작동 화면

@ningpop ningpop added documentation Improvements or additions to documentation 🍎과제🍎 labels Oct 29, 2021
@ningpop ningpop self-assigned this Oct 29, 2021
@ningpop ningpop changed the title Feature/12 week3 [FEAT] iOS 3차 과제 Oct 29, 2021
Copy link
Member

@Taehyeon-Kim Taehyeon-Kim left a comment

Choose a reason for hiding this comment

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

LGTM ✨ 사실 테이블뷰랑 컬렉션뷰랑 잘 사용해주셔서 완벽한 것 같습니다~ 이슈 같은 경우는 지난번 금잔디 미팅에서 해결했었죠? Constants로 아예 이미지들을 관리하는 것도 좋을 것 같아보이네요! 그럼 이슈를 찾을 때 Constants 파일부터 체킹을 시작해 이미지 이름을 올바르게 정해놓았는지 찾기 편할 것 같아요. 고생하셨습니다~~

Comment on lines 15 to 16
var homeVideoList: [HomeVideo] = []
var homeShortsVideoList: [HomeShortsVideo] = []
Copy link
Member

Choose a reason for hiding this comment

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

해당 클래스에서만 사용되는 변수라면 private접근제어자를 이용해서 감싸주는 것도 좋아보입니다! 얘네 같은 경우는 어쨌든 데이터이기 때문에 실수로 외부에서 접근해서 변경되면 안 될 것 같아서요! 참고만 해주세요~

Choose a reason for hiding this comment

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

저도 한 수 배워갑니당~

Copy link
Member Author

Choose a reason for hiding this comment

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

아.. private!!! OOP강연을 해놓고 이걸 놓치다니ㅋㅋㅋ큐ㅠㅠ 감사합니다~

Comment on lines +97 to +99
func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, insetForSectionAt section: Int) -> UIEdgeInsets {
return UIEdgeInsets.zero
}
Copy link
Member

Choose a reason for hiding this comment

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

그냥 생각이 든 건데 다음과 같이 쓸 수도 있어요!

return .zero

Copy link
Member Author

Choose a reason for hiding this comment

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

오... 단축형... 어떤 문법에 의해서 그렇게 되는지 혹시 참고자료나 키워드가 있을까요?

Copy link

@eunseo5355 eunseo5355 left a comment

Choose a reason for hiding this comment

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

코드가 깔끔해서 보기 좋았습니다~!👍
수고 많으셨어요!

Copy link
Member

@EunHee-Jeong EunHee-Jeong left a comment

Choose a reason for hiding this comment

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

과제하시느라 수고 많으셨습니다~! 코드가 깔끔해서 읽기 편했어요 👍

Comment on lines 23 to 26
homeVideoTableView.dataSource = self
homeVideoTableView.delegate = self
homeShortsCollectionView.dataSource = self
homeShortsCollectionView.delegate = self
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 메서드로 모아서 호출하는게 더 좋다고 하네요 ㅎㅎ

Choose a reason for hiding this comment

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

넴 set~ 으로 묶어서 하는 것을 추천하시는 분들이 많더라고용

Copy link
Member Author

Choose a reason for hiding this comment

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

마음이 급해서 저리 하였네유... 다음에는 좀 더 깨끗한 코드로 돌아오겠습니다!

@ningpop ningpop merged commit 35eb880 into main Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation 🍎과제🍎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants