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

Feature/week3 과제 구현 중 .. #6

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Feature/week3 과제 구현 중 .. #6

wants to merge 9 commits into from

Conversation

HEHEEUN
Copy link
Collaborator

@HEHEEUN HEHEEUN commented Nov 1, 2024

구현 완료 항목

  • 헤더 뷰 생성
  • 금융 페이지 테이블 뷰 생성

Simulator Screenshot - iPhone 16 Pro - 2024-11-01 at 17 27 22

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-01.at.20.51.23.mp4

Copy link

@juri123123 juri123123 left a comment

Choose a reason for hiding this comment

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

코드 잘 봤습니다 1!!! 고생많으셨습니다 1!!!!

protocol DetailDelegate: AnyObject {
func dataBind(nickname: String) // 다음화면에서 이전화면으로 Data
}
class DetailViewController_Closer: UIViewController {

Choose a reason for hiding this comment

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

네이밍을 할 때 camel case로 하면 좋을 것 같아요 !!

private func setUpButton() {
versionInfoView.updateRecordButton.addTarget(self, action: #selector(updateRecordButtonTapped), for: .touchUpInside)

appScoreView.allReviewButton.addTarget(self, action: #selector(allReviewButtonTapped), for: .touchUpInside)

Choose a reason for hiding this comment

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

delegate를 사용하면 좋을 것 같아요 !

Copy link

@Johyerin Johyerin 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

Choose a reason for hiding this comment

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

.DS_Store 파일은 당장은 문제가 되지 않더라도 추후에 깃에서 충돌을 유발하게 될 수도 있기 때문에 올라가지 않게 주의해주시는 게 좋습니다!

Comment on lines +56 to +61
scrollView.snp.makeConstraints {
$0.top.equalToSuperview()
$0.bottom.equalToSuperview()
$0.leading.equalToSuperview()
$0.trailing.equalToSuperview()
}
Copy link

Choose a reason for hiding this comment

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

$0.edges.equalToSuperview()로 적어줄 수 있을 것 같아용

}
contentView.snp.makeConstraints {
$0.edges.equalToSuperview()
$0.width.equalTo(scrollView.snp.width)
Copy link

Choose a reason for hiding this comment

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

contentViewscrollView에 속하기 때문에 equalToSuperview()로 적어줘도 좋을 것 같습니닷~!

}
titleView.snp.makeConstraints {
$0.height.equalTo(150)
$0.leading.trailing.equalToSuperview()
Copy link

Choose a reason for hiding this comment

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

$0.horizontalEdge.equalToSuperview() 로도 적어줄 수 있을 것 같아욥!

Comment on lines +8 to +10
import SnapKit
import UIKit
import Then
Copy link

Choose a reason for hiding this comment

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

퍼스트 파티 프레임워크인 UIKit을 가장 먼저 임포트해주면 더 보기에 좋을 것 같다는 의견 남깁니당

Comment on lines +57 to +58
$0.top.equalToSuperview().offset(20)
$0.leading.equalToSuperview().offset(20)
Copy link

Choose a reason for hiding this comment

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

superview를 기준으로 잡을 때에는 inset 사용을 추천드립니다!

Copy link
Member

@mcrkgus mcrkgus left a comment

Choose a reason for hiding this comment

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

너무 고생하셨습니당!!

setLayout()
}

private func setUI() {
Copy link
Member

Choose a reason for hiding this comment

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

접근제어를 private로 하신 이유가 무엇인가용

Comment on lines +36 to +37
$0.delegate = self
$0.dataSource = self
Copy link
Member

Choose a reason for hiding this comment

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

delegate나 dataSource를 채택하는 부분은 따로 메소드화 시켜도 좋을 것 같아요 !
지금은 tableview 하나만 채택하면 되지만, 나중에 데이터 전달이 많아지게 될 경우에는 한 메소드에서 관리하는게 훨씬 편합니당


private func setLayout() {
chartTableView.snp.makeConstraints {
$0.leading.trailing.top.bottom.equalToSuperview()
Copy link
Member

Choose a reason for hiding this comment

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

요고는
$0.edges.equalToSuperview()
로도 쓸 수 있습니다


class ChartCell: UITableViewCell {

private lazy var iconImageView = UIImageView()
Copy link
Member

Choose a reason for hiding this comment

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

lazy 키워드를 사용하신 이유가 무엇인가용

Comment on lines +64 to +69
func tableView(_ tableView: UITableView, heightForHeaderInSection section: Int) -> CGFloat {
let sectionData = sectionList[section]
let headerHeight: CGFloat = sectionData.title.isEmpty ? 0 :
sectionData.subtitle.isEmpty ? 60 : 75
return headerHeight
}
Copy link
Member

Choose a reason for hiding this comment

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

좋네용

Copy link

@sozohoy sozohoy left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 가현님이 이미 코드리뷰를 잘 남겨주셔서..
또한, 해당 과제와 상관없는 커밋이 같이 껴서 들어오는 이유에 대해서 알아보세요!

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.

5 participants