Skip to content

Conversation

@Naknakk
Copy link
Contributor

@Naknakk Naknakk commented Jun 11, 2024

⭐️Issue

close #144

🌟Motivation

  • NovelDetail Header UI 구현

🌟Key Changes

  • UI 위주 구현이며, 노벨 커버 이미지를 탭 하면 커다란 이미지가 나오는 정도의 구현만 있습니다.
  • 중간의 별점 부분은 읽기 상태 선택 뷰로 변경될 예정이라, 디자인 미정으로 두었습니다.
  • Repository 패턴을 활용하기 위해, NovelDetailRepository 프로토콜을 따르는 TestRepository를 이용해 목업 데이터를 받아왔습니다. 이후 서버 통신을 통해 받아오는 레포지토리로 변경하기만 하면 VC의 코드 변경없이 적용 가능!
    protocol NovelDetailRepository {
    func getNovelBasic(novelId: Int) -> Observable<NovelDetailBasicResult>
    }
    struct TestDetailRepository: NovelDetailRepository {
    func getNovelBasic(novelId: Int) -> Observable<NovelDetailBasicResult> {
    return Observable.just(
    NovelDetailBasicResult(userNovelID: nil,
    novelTitle: "여자친구로 삼으려고 학생회장을 꼭 닮은 여자아이를 연성했다가 내가 하인이 됐습니다",
    novelImage: "ImgNovelCoverDummy",
    novelGenres: ["romanceFantasy", "romance"],
    novelGenreURL: "icGenreLabelRfDummy",
    isNovelCompleted: false,
    author: "이보라",
    interestCount: 203,
    novelRating: 4.4,
    novelRatingCount: 153,
    feedCount: 52,
    userNovelRating: 5.0,
    readStatus: "READING",
    startDate: nil,
    endDate: nil,
    isUserNovelInterest: true)

🌟Simulation

iPhone SE 3rd iPhone 13 mini iPhone 15 Pro

🌟To Reviewer

  • SceneDelegate는 Merge할 때 수정할게요!
  • TestViewController 도 마찬가지로 Merge할 때 삭제하겠습니다!
  • 변경점이 엄청 많아 보이는데,, Trash로 옮기는 작업 때문에 그런거라 겁먹지 않아도 될..겁니다 .. ㅎㅎ

Naknakk added 30 commits April 27, 2024 01:37
@Naknakk Naknakk marked this pull request as ready for review June 19, 2024 15:49
Copy link
Member

@Guryss Guryss 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 +15 to +18
protocol DetailModuleFactory {
func makeDetailViewController(novelId: Int) -> UIViewController
}

Copy link
Member

Choose a reason for hiding this comment

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

객체들간의 의존성을 줄이기 위한 Factory Method Pattern!
저도 꼭 ............. 해보겠습니다 >__<

Comment on lines +228 to +229
static let complete = " · 완결작 · "
static let inSeries = " · 연재중 · "
Copy link
Member

Choose a reason for hiding this comment

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

스페이스바 개수까지 맞춘 당신은 ................................ 섬세왕

static let novelAuthor = "작품 작가"
static let novelGenre = "작품 장르"
static let novelInterestCount = "0"
static let novelRatingCount = "0.0 (0)"
Copy link
Member

Choose a reason for hiding this comment

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

W3;
추후에 서버에서 받아온다면 해당 값은 두개로 다시 나뉘어져야 할 것 같네용!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이곳은 로딩중에 띄워줄 초기 값을 설정해둔 거라 한가지로 해버렸는데요 ..!
novelRating = "0.0" 과 novelRatingCount = 0 으로 로딩중 값도 나눠주는게 좋을까요?

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
Contributor Author

@Naknakk Naknakk Jun 23, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-06-23 at 13 20 09
그런 의도라라면 서버에서 받는 값도 이렇게 하나의 Label에 보간법으로 넣어주고 있어서 히히 괜찮을 것 같다는 생각!!

Copy link
Member

Choose a reason for hiding this comment

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

넹~! 알겠솝니당 ㅎ 굿

let userNovelID: Int?
let novelTitle, novelImage: String
let novelGenres: [String]
let novelGenreURL: String
Copy link
Member

Choose a reason for hiding this comment

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

W2;
소설 장르 이미지 Url 이라면 novelGenreImage 가 어떤가요?
위에 소설 이미지는 novelImage이고 장르 이미지는 URL이라 약간 네이밍이 헷갈리네용..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헛 서버에서 주는 원래 변수명이 novelGenreURL이었던 걸로 기억해요! 그래서 그냥 그대로 해두었었네요 ..
지금은 이야기해준대로 novelGenreImage로 바뀌었네요!
아무튼 변수명은 Image로 두고 코딩 키를 URL로 해두었어도 되는데 그쵸.. ㅋㅋㅋ 좋은 지적입니다!!

Copy link
Member

Choose a reason for hiding this comment

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

깔깔 그렇군용 .... 레포지토리 패턴을 활용하니까 우린 우리의 길을 걸어도 될듯! ㅎ

let novelRating: Float
let novelRatingCount, feedCount: Int
let userNovelRating: Float
let readStatus: String
Copy link
Member

Choose a reason for hiding this comment

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

W2;
readStatus도 사용자가 로그인하지 않았을 때에는 Nullable하다고 되어있는데, 옵셔널 처리 해야하지 않을까요?

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

@Naknakk Naknakk Jun 22, 2024

Choose a reason for hiding this comment

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

헛 이 사항도 6월 6일에 노션에서 수정이 있었던 부분이네요 ! 미쳐 발견하지 못했었는데 반영하겠습니다!
그나저나, 이렇게 Entity의 수정이 생기면서 전체 코드의 변경점이 생기는걸 보니 .. Entity와 DTO의 분리가 필요한 이유를 체감할 수 있네요...!

Copy link
Member

Choose a reason for hiding this comment

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

오호호 그렇네용 .. 수정해준다면 저도 그렇게 ! 따라가도록 ! 까깔

Comment on lines +62 to +63
$0.width.lessThanOrEqualToSuperview().inset(20)
$0.height.lessThanOrEqualToSuperview().inset(60)
Copy link
Member

Choose a reason for hiding this comment

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

W2;
우와 해당 내용은 superView의 width에서 inset을 20보다 작거나 같게, superView의 height의 크기에서 60보다 작거나 같게 한다는 레이아웃 정의인가요?

lessThanOrEqualToSuperview() 해당 속성을 사용한 건 기기대응을 위한 것???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 맞습니다! 노벨 커버 이미지를 탭했을때, 사용자가 일반적으로 웹소설 커버 이미지가 원본 사이즈로 나오기를 기대할거라 생각했기 때문에 적용한 부분입니다!
만약 원본 이미지가 세로로 너무 길다면, scaleAspectFill로 두었기 때문에 가로 기준으로 채워지면서 세로가 잘릴 것 같고..
이미지가 가로로 너무 길다면 세로 기준으로 채워지면서 가로가 잘릴 것 같다고 판단했기 때문에
가로 세로 길이의 최대값을 두어 이미지가 짤리지 않도록 했습니다!!

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
Member

Choose a reason for hiding this comment

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

W2
크기를 임의로 지정하고 우선순위를 추가하지 않은 이유가 있을까요 ?
.priority() 이걸 사용해도 좋을 것 같아요!

Copy link
Contributor Author

@Naknakk Naknakk Jun 24, 2024

Choose a reason for hiding this comment

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

우선순위를 주기 애매한 상황이라 그렇습니다!
가로에 우선순위를 둔다면, 아래와 같이 세로로 긴 이미지의 경우 가로 기준으로 쭉 늘어나면서 세로가 화면을 뚫어버리기 때문입니다...

Screenshot 2024-06-24 at 15 19 58

반대로 세로에 우선순위를 둔다면, 가로로 긴 이미지의 경우 세로 기준으로 쭉 늘어나면서 가로가 화면을 넘어갑니다..!
그래서 임의로 가로 세로의 최대값을 주고, 두 부분 중 어느 한 곳의 최대값에 도달하면 멈출 수 있게 했습니당..!

Copy link
Member

Choose a reason for hiding this comment

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

💟👍🏻


dismissButton.snp.makeConstraints {
$0.size.equalTo(44)
$0.top.equalTo(safeAreaLayoutGuide.snp.top).offset(-44)
Copy link
Member

Choose a reason for hiding this comment

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

W2;
하나 궁금한 점이 있는데, offset을 주지 않으면 노치 바로 위에 dismissButton이 위치할 줄 알았는데 그렇지가 않네요...?
왜 offset(-44)를 줬는지 알려주실 수 있나용!

Copy link
Contributor Author

@Naknakk Naknakk Jun 22, 2024

Choose a reason for hiding this comment

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

저도 진짜 그럴줄 알았습니다.. 근데 분명 네비게이션 바 히든 -> LargeCoverImageView가 Hidden해제 되는데도 네비게이션 바가 존재하는 기준으로 safeAreaLayoutGuid를 잡더라구요..?? (현재 코드는 반대 순서이긴 하지만요..)
그래서 네비게이션 바 높이만큼 오프셋을 -44로 주었습니다.
SE나 미니나 상관없이 네비게이션 바 자체 높이는 동일하니 기기대응이 가능할 거라는 생각으로 superview의 top에서 조금 내리는 식으로 주는게 아니라, 세이프에리어에서 -44로 주었습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오잉 근데 ;; 현재 작업중인 브랜치에서 해결됐어요. offset 없애니 자연스럽게 StatusBar 바로 아래에 위치하게 되었습니다... 왜인지는 모르겠어요 ...

}

private func setHierarchy() {
self.addSubviews(stackView)
Copy link
Member

Choose a reason for hiding this comment

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

s 제거

Comment on lines 43 to 46
let detailBasicData: Observable<NovelDetailBasicResult>
let scrollContentOffset: Driver<CGPoint>
let showLargeNovelCoverImage: Driver<Bool>
let backButtonDidTap: Observable<Void>
Copy link
Member

Choose a reason for hiding this comment

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

W1;
image

컨벤션 확인 부탁드려용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 output 컨벤션을 완전 잊었네요 고맙습니다

Comment on lines 77 to 82
return Output(
detailBasicData: novelDetailBasicData.asObservable(),
scrollContentOffset: scrollContentOffset.asDriver(),
showLargeNovelCoverImage: showLargeNovelCoverImage.asDriver(),
backButtonDidTap: backButtonDidTap
)
Copy link
Member

Choose a reason for hiding this comment

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

viewModel의 Properties로 따로 빼준 뒤에 return Output에서 연결해주는 이유가 따로 있나요?

전 viewModel의 properties를 따로 만들지 않으려고 했고 바로 output내의 프로퍼티들과 연결지었거든요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이렇게 해둔 이유는 Output으로 주는 값 자체가 Relay나 Subject등 rx 객체가 아니게 하고 싶었기 때문입니당!!

Output은 뷰가 단지 바라보고 있는 값일 뿐 뷰가 값을 직접 넘기는 일은 없잖아용..? 그래서 그 의미를 명확하게 하고싶어서 뷰모델에 객체를 두고, Driver, Observable 처럼 관찰만 할 수 있는 값으로 넘겨주는게 mvvm 의미상 더 좋지 않을까..?하구 생각해서 이렇게 해두었답니다!!
private를 붙이는 느낌.. ? 혹은 let과 var 느낌 ..?? 아무튼 output 자체에 accept나 onNext로 값을 넘길 수 있다는 정보가 숨겨지면 좋겠다구 생각했어요!

Copy link
Member

Choose a reason for hiding this comment

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

오호... 너무 좋은 생각인듯?

아직 사실 저도 부족한 부분이 많은 것 같아서 이 부분은 나중에 같이 얘기해봐도 좋을 것 같아요 🙌 저도 좀 공부해서.. 다시 얘기해봅시댜

@Naknakk Naknakk requested a review from Guryss June 23, 2024 04:51
Copy link
Member

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

@ena-isme ena-isme left a comment

Choose a reason for hiding this comment

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

수고하셨숨둥 🙆🏻‍♀️
이미지 확대되었을 때 뒷 배경 Tap 할 시 dismiss 되는 기능은 추가 안된걸까요 ?

Comment on lines +53 to +55
novelGenreBookmarkBackgroundImageView.do {
$0.image = .icGenreBackground
$0.contentMode = .scaleAspectFit
Copy link
Member

Choose a reason for hiding this comment

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

W2
요 부분 이미지 안쓰고는 어려운가용 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

가능하긴 한데 이미지 그냥 넣는 것 보다는 좀 더 복잡하게 해야할 것 같네요 ..!
다음에 시간이 되면 직접 구현하는 걸로 한번 해볼게요!!

Comment on lines +77 to +87
private func createStars() -> [UIImageView] {
return (0..<5).map { _ in
let starImageView = UIImageView().then {
$0.isUserInteractionEnabled = true
$0.image = .icStarEmpty
$0.contentMode = .scaleToFill
$0.clipsToBounds = true
}
return starImageView
}
}
Copy link
Member

Choose a reason for hiding this comment

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

W3
이렇게 만들어주니까 깔끔쓰하고만 ,,

Comment on lines +62 to +63
$0.width.lessThanOrEqualToSuperview().inset(20)
$0.height.lessThanOrEqualToSuperview().inset(60)
Copy link
Member

Choose a reason for hiding this comment

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

W2
크기를 임의로 지정하고 우선순위를 추가하지 않은 이유가 있을까요 ?
.priority() 이걸 사용해도 좋을 것 같아요!

Comment on lines +72 to +75
$0.setCustomSpacing(-148, after: bannerBackgroundImageView)
$0.setCustomSpacing(20, after: novelCoverImageButton)
$0.setCustomSpacing(26, after: novelInfoView)
$0.setCustomSpacing(6, after: novelEstimateButton)
Copy link
Member

Choose a reason for hiding this comment

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

W3
오호 이런게 있었군요 배우고 갑니당 ,,,


import UIKit

import Kingfisher
Copy link
Member

Choose a reason for hiding this comment

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

W2
Kingfiser 는 UI 구현 부분이라 View 에서 사용해주는 건 어떨까용 ?

Copy link
Contributor Author

@Naknakk Naknakk Jun 24, 2024

Choose a reason for hiding this comment

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

넵넵 좋습니다 근데 사용하려고 추가한게 아니라 그냥 .. 복사하다보니 들어가있었네요ㅋㅋㅋ ㅜ 삭제할게요!
근데 이 뷰컨은 테스트용으로 간단히 만든거라 머지할 때 삭제할 예정이에요! PR에 적어두었던 내용이라 참고해서 봐주면 고맙겠습니당~~

$0.setTitle("Next", for: .normal)
}

let disposeBag = DisposeBag()
Copy link
Member

Choose a reason for hiding this comment

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

W1
private 로 사용해주는 게 더 좋을 것 같아요~!

@Naknakk
Copy link
Contributor Author

Naknakk commented Jun 24, 2024

수고하셨숨둥 🙆🏻‍♀️ 이미지 확대되었을 때 뒷 배경 Tap 할 시 dismiss 되는 기능은 추가 안된걸까요 ?

네엡 명확하게 X 버튼이 있어서 추가하진 않았는데, 한번 기획쪽에 물어보고 필요하다면 추가할게요!
ㄴ필요하다고 해서 추가했습니다!!

@Naknakk Naknakk requested a review from ena-isme June 24, 2024 08:04
@Naknakk Naknakk merged commit 479178c into main Jun 24, 2024
@Naknakk Naknakk deleted the Design/#144-NovelDetailView branch June 24, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Design] NovelDetail Header UI 구현

4 participants