-
Notifications
You must be signed in to change notification settings - Fork 1
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
BookCover 엔티티 & MHBook 뷰 구현 #47
Conversation
피그잼의 BookCover와 조금 다를 수 있습니다..! favorite 같은 프로퍼티는 쓰이지 않아, 정말 MHBook에만 필요한 것만 넣었습니다
컨테이너 뷰 안에 아래의 내용을 넣었습니다 - 책 겉면 (대영현 디자인) - 책 제목 - 대상 이미지 - 유저 출판소 이름
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.
Collection Flow는 따로 올리기로 했습니다
MHBook만 먼저 올렸는데, 모델부터 해서 아직 부족한 게 많네요
리뷰하면서 고쳐나가겠습니다 ~~
오늘 무지성 회의만 했는데.. 굿나잇입니다 여러분 🌙
// MARK: - Configuration | ||
func configure(with bookCover: BookCover) { | ||
titleLabel.text = bookCover.title | ||
bookImageView.image = bookCover.bookType.image | ||
targetImageView.image = UIImage(systemName: "person") | ||
if let publisher = UserDefaults.standard.object(forKey: Constant.houseNameUserDefaultKey) as? String { | ||
// TODO: User 모델 만들어지면 파라미터로 출판소 이름 넘겨주기 | ||
publisherLabel.text = publisher | ||
} | ||
} |
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.
CollectionViewCell에서 사용되어야 하기 때문에 다음과 같이 configure로 설정해주었습니다
CollectionViewDatasource에서 Reuse 되면, cell.configure(with: 모델) 호출
cell.configure(with:)에서 해당 메소드를 호출하게끔 할 생각이긴 합니다..!
// TODO: 위치 변경 고려해보기 | ||
extension BookType { | ||
var image: UIImage { | ||
switch self { | ||
case .blue: .blueBook | ||
case .beige: .beigeBook | ||
case .green: .greenBook | ||
case .orange: .orangeBook | ||
case .pink: .pinkBook | ||
@unknown default: | ||
fatalError("등록되지 않은 책 색상입니다.") | ||
} | ||
} | ||
} |
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.
Domain에 있는 모델이지만 Presentation에서 위처럼 연산 프로퍼티 접근이 가능하면 용이할 것 같아 추가해봤습니다
괜찮다면..? 해당 코드가 어디 위치에 있는게 좋을까요
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.
[추가] 아래 코드는 필수로 구현해줘야 했습니다!
책이 안 보이면 강제종료 되는게 맞다 판단하여 fatalError 그대로 냅뒀는데 다른 아이디어 있으신 분 있으면 알려주세욥
컴파일러가 띄워준 에러에 자동완성 Fix가 @.unkown default였는데
그냥 늘 쓰던 default였군요, 수정했습니당
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.enum인데 default가 수행되나욤?
- 저는 처음에 ViewModel에서 대응할까 했는데, 모듈로 분리되어있으니까 Presentation에서 extension을 만들어도 될것같긴합니다.!
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.
�엇 저는 BookColor의 extension으로 구현하기보단 viewModel에서 대응하는게 더 좋아보입니당..! 그럼 default를 따로 만들지 않아도 될 것 같아요 ..
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.
제 생각에도 default 수행 안될 거 같은데,
다른 모듈에 있는 걸 extension으로 열어서 그런지 @unknown default
를 해주지 않으면 에러가 발생했습니다 !
다른 모듈에서 접근 제어자 때문에 해당 extension 못 사용하므로
Domain에 있는 엔티티의 확장을 해도 된다고 판단했습니다..!
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.
@frozen
을 사용해 타입을 고정시켜도 @unknown default
를 작성해야하는 문제가 발생할까요?
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.
컴파일러가 띄워준 에러에 자동완성 Fix가 @.unkown default였는데
그냥 늘 쓰던 default였군요, 제거하고 default로 수정했습니다 !!
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.
효준님 늦은시간 고생 많으셨습니다 ㅠㅠ 🙇♂️ 이렇게 Book도 만들어주시고.... 감사히 잘 쓰겠습니다?!헿... 정말 고생 많으셨어요...
containerView.fillSuperview() | ||
bookImageView.fillSuperview() |
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.
P2. ContainerView의 역할이 모호해보입니다... 이렇게 할꺼면 MHBook이 ContainerView의 역할을 수행하거나 MHBook이 UIImageView를 상속받아도 될것같은데 이렇게 구현하신 이유가 있으신지 궁금합니다!
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.
인정합니다 !
컨테이너 뷰 제거해서 다시 올렸습니당
// TODO: 위치 변경 고려해보기 | ||
extension BookType { | ||
var image: UIImage { | ||
switch self { | ||
case .blue: .blueBook | ||
case .beige: .beigeBook | ||
case .green: .greenBook | ||
case .orange: .orangeBook | ||
case .pink: .pinkBook | ||
@unknown default: | ||
fatalError("등록되지 않은 책 색상입니다.") | ||
} | ||
} | ||
} |
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.enum인데 default가 수행되나욤?
- 저는 처음에 ViewModel에서 대응할까 했는데, 모듈로 분리되어있으니까 Presentation에서 extension을 만들어도 될것같긴합니다.!
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.
수고많으셨습니당 .ᐟ.ᐟ
scaleToFill보다 scaleAspectFill을 하고 imageView의 크기만큼 clipsToBounds 해주면 더 좋을 것 같습니다 .ᐟ.ᐟ (비율이 안맞는게 조금 외관상 보기가.. 안좋은 느낌..?)
public enum BookType { | ||
case beige | ||
case blue | ||
case green | ||
case orange | ||
case pink | ||
} |
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.
UIImage에 대한 건데 Color라는 이름을 쓰기가 애매했습니다..!
아래처럼 사용돼도 괜찮나요 ? (개인적) 전 Type이 나을 거 같은데 어떠신가용
extension BookColor {
var image: UIImage {
return .pinkColor
}
}
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.
BookColor가 저희의 비즈니스 모델이니까 괜찮지 않을까요?? 전 개인적으로 괜찮아보이긴합니다 .ᐟ.ᐟ
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.
인정합니다 수정할게욥
// TODO: 위치 변경 고려해보기 | ||
extension BookType { | ||
var image: UIImage { | ||
switch self { | ||
case .blue: .blueBook | ||
case .beige: .beigeBook | ||
case .green: .greenBook | ||
case .orange: .orangeBook | ||
case .pink: .pinkBook | ||
@unknown default: | ||
fatalError("등록되지 않은 책 색상입니다.") | ||
} | ||
} | ||
} |
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.
�엇 저는 BookColor의 extension으로 구현하기보단 viewModel에서 대응하는게 더 좋아보입니당..! 그럼 default를 따로 만들지 않아도 될 것 같아요 ..
// MARK: - Configuration | ||
func configure(with bookCover: BookCover) { | ||
titleLabel.text = bookCover.title | ||
bookImageView.image = bookCover.bookType.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.
저는 여기서 case별로 나눠서 image를 적용해줘도 된다.. 생각합니당.. 뭔가 model의 extension에 computed property가 들어가는게 어색..한 것 같다..? 는 의견입니당..
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.
고생하셨슴 당당 ~~
// TODO: 위치 변경 고려해보기 | ||
extension BookType { | ||
var image: UIImage { | ||
switch self { | ||
case .blue: .blueBook | ||
case .beige: .beigeBook | ||
case .green: .greenBook | ||
case .orange: .orangeBook | ||
case .pink: .pinkBook | ||
@unknown default: | ||
fatalError("등록되지 않은 책 색상입니다.") | ||
} | ||
} | ||
} |
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.
@frozen
을 사용해 타입을 고정시켜도 @unknown default
를 작성해야하는 문제가 발생할까요?
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.
너무너무 수고 많으셨습니다 ~.ᐟ.ᐟ 👍👍👍
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.
대장님 고생 많으셨어요❕❕
#️⃣ 연관된 이슈
📝 작업 내용
📒 리뷰 노트
BookCover 엔티티
Presentation 단에서 확장하여 구현했습니다.
MHBook 뷰
우리 앱의 경우 사진 선택할 때 자르기 기능이 주어지니 scaleToFill로 정했습니다 ! (맞는지 모르겠네요)
📸 스크린샷