-
Notifications
You must be signed in to change notification settings - Fork 1
[#258]만든 사람들 페이지 수정 #277
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: develop
Are you sure you want to change the base?
[#258]만든 사람들 페이지 수정 #277
Conversation
- 만든 사람들 이미지 변경 - 그라데이션 설정 #258
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.
수고하셨습니다. 리뷰 달아놓았는데 읽어보시고 답변주세요~!
make.width.equalTo(276) | ||
make.height.equalTo(1770) |
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.
해당 코드는 setLayout()
함수 내에 있는 것이 더 적절해보입니다!
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.
넵 수정하겠습니다!
scrollView.snp.makeConstraints { make in | ||
make.edges.equalToSuperview() // 화면 전체에 ScrollView | ||
} | ||
|
||
contentView.snp.makeConstraints { make in | ||
make.edges.equalTo(scrollView) // ScrollView 내부에 맞춤 | ||
make.width.equalToSuperview() // 가로 크기는 화면 크기와 동일 | ||
make.bottom.equalTo(creatorsImageView.snp.bottom).offset(52) // 높이 지정 | ||
} |
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.
해당 부분에 대한 주석은 없어도 될 것 같은데 어떻게 생각하시나요? @Hrepay
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 승인하실 필요없어요!
제가 다른 프로젝트를 하면서 요즘 들어 느꼈는데, 사소한 것이어도 결국 병합하게 되니까 바로바로 개선사항을 반영해야겠더라구요.
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.
불필요한 주석 모두 제거하겠습니다!
@@ -31,9 +31,9 @@ class CreatorViewController: BaseViewController { | |||
|
|||
override func setLayout() { | |||
creatorsView.snp.makeConstraints { make in | |||
make.top.equalToSuperview().inset(103) | |||
make.top.equalToSuperview().inset(66) | |||
make.leading.trailing.equalToSuperview().inset(24) |
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.
이렇게도 사용할 수 있어요!
make.leading.trailing.equalToSuperview().inset(24) | |
make.horizontalEdges.equalToSuperview().inset(24) |
UIColor(red: 184/255, green: 228/255, blue: 255/255, alpha: 1.0).cgColor, | ||
UIColor(red: 199/255, green: 255/255, blue: 227/255, alpha: 1.0).cgColor |
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.
일회성으로 사용하는 컬러는 해당 파일에서 선언하여 사용하는 것이 좋을까요? 아님 디자인 시스템에 포함시키는 것이 좋을까요? @Hrepay
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.
그럼 일회성 컬러여도 디자인 시스템에 추가하는 방향으로 통일합시다.
@@ -1,7 +1,7 @@ | |||
{ | |||
"images" : [ | |||
{ | |||
"filename" : "Creators.png", | |||
"filename" : "Creators3.png", |
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.
기존에 사용하고 있던 Creator
파일과 중복되어 해당 파일명을 사용하신 것 같은데, 기존에 있던 이미지파일 삭제하셔도 됩니다!
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.
불필요한 주석은 제거해서 다시 개선해주셨으면 합니다! 작업하신 건 너무 고생많았어요!
- 리뷰 수정사항 변경 #258
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.
수고하셨습니다!
let scrollView: UIScrollView = UIScrollView().then { | ||
$0.showsVerticalScrollIndicator = false | ||
$0.contentInsetAdjustmentBehavior = .never | ||
} | ||
|
||
let contentView: UIView = UIView().then { | ||
$0.backgroundColor = .clear | ||
} | ||
|
||
private let creatorsImageView: UIImageView = { | ||
let imageView = UIImageView() | ||
imageView.image = EATSSUDesignAsset.Images.creators.image | ||
imageView.contentMode = .scaleAspectFit | ||
imageView.snp.makeConstraints { make in | ||
make.width.equalTo(342) | ||
make.height.equalTo(689) | ||
} | ||
return imageView | ||
}() |
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.
UIColor(red: 184/255, green: 228/255, blue: 255/255, alpha: 1.0).cgColor, | ||
UIColor(red: 199/255, green: 255/255, blue: 227/255, alpha: 1.0).cgColor |
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.
피그마 보니까 배경 그라데이션에 대한 색인 것 같은데, 이 페이지는 앞으로도 쭉 가져갈 것 같아서 저는 포함시키는 것도 좋은 방법인 것 같습니다.
#️⃣ 관련 이슈
Resolved #258
💡작업 내용
💬리뷰 요구사항(선택)