레시피 상세화면 UI를 정의해 보았습니다. #12

GeonH0 · 2024-07-03T03:15:19Z

다른 화면면에도 사용할 NaviagtionBar를 정의해 보았습니다.
Mapper의 이름을 RecipeItem에서 Detail로 변경했습니다.
DetailViewController를 정의했습니다.
View를 정의했습니다.

f-lab-barry · 2024-07-03T14:30:52Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/RecipeDetailViewModel.swift

+    let name: String
+    let description: String
+    let imageUrls: [URL]
+    let isLikedByCurrentUser: Bool


뷰모델에서는 좀 더 UI에 맞는 네이밍으로 가는건 어때요?
도메인에서의 디테일한 이름을 그대로 담기보다는 isLiked 정도로 충분해보여요.

[9881443],[fa15e8f] 수정했습니다

앞에 다 대문자로 시작하는데 카멜케이스로 고쳐주시면 좋을것같아요

f-lab-barry · 2024-07-03T14:31:08Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/RecipeDetailViewModel.swift

+    let description: String
+    let imageUrls: [URL]
+    let isLikedByCurrentUser: Bool
+    let createdAt: Date


이 값은 뷰에서 어떻게 쓰이는거에요?

description 은 설명
imgarUrls는 레시피의 이미지
isLikedByCurrentUser는 추후만들수도 있는 좋아요 유무
createAt는 현재는 사용하고 있지 않고 있어요

사용하지 않는건 제거해주세요

[f8b8c75] 수정했습니다

f-lab-barry · 2024-07-03T14:32:01Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+import UIKit
+import Kingfisher


써드파티와 퍼스트 파티 사이는 한 줄 띄어주세요

[d508f7e] 수정했습니다

f-lab-barry · 2024-07-03T14:33:04Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+
+    private let scrollView = UIScrollView()
+    private let pageControl = UIPageControl()
+    private let recipenameLabel = UILabel()


Suggested change

private let recipenameLabel = UILabel()

private let recipeNameLabel = UILabel()

[092afac] 수정했습니다

f-lab-barry · 2024-07-03T14:33:34Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+    private let scrollView = UIScrollView()
+    private let pageControl = UIPageControl()
+    private let recipenameLabel = UILabel()
+    private let recipedescriptionLabel = UILabel()


Suggested change

private let recipedescriptionLabel = UILabel()

private let recipeDescriptionLabel = UILabel()

[1b83f41] 수정했습니다

f-lab-barry · 2024-07-03T14:34:05Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+    private let recipenameLabel = UILabel()
+    private let recipedescriptionLabel = UILabel()
+    private let photoIndexLabel = UILabel()
+    private var recipeimageUrls: [URL] = []


Suggested change

private var recipeimageUrls: [URL] = []

private var recipeImageUrls: [URL] = []

[a78dd53] 수정했습니다

f-lab-barry · 2024-07-03T14:36:45Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+            recipedescriptionLabel.trailingAnchor.constraint(equalTo: trailingAnchor, constant: -20)
+        ])
+
+        recipenameLabel.font = .systemFont(ofSize: 24, weight: .bold)


저희 폰트 따로 정의해서 쓰기로 하지 않았나요?
https://github.com/f-lab-edu/HomeCafeRecipes/blob/main/HomeCafeRecipes/HomeCafeRecipes/Resources/Fonts.swift

[1f1c396]
적용 시켰는데 혹시 안올라 간건가요?

뒤 커밋에서 적용하신거 확인했어요~ 이런 부분에 fixup이나 squash로 커밋을 정리해서 올리면 좋겠네요 😄
(대부분 커밋 순서대로 리뷰한다는 것을 감안해주세요. file Changes로 보면 수정이 되어있겠지만 커밋단위는 되어있지 않아요.)

넵 다음부터는 커밋 정리후 올리겠습니다!

f-lab-barry · 2024-07-03T14:38:08Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+    func configure(with viewModel: RecipeDetailViewModel) {
+        recipenameLabel.text = viewModel.name
+        recipedescriptionLabel.text = viewModel.description
+        recipeimageUrls = viewModel.imageUrls


recipeimageUrls를 내부에 들고있는 이유가 따로 있을까요?
스크롤뷰를 구성하는데 바로 전달해서 사용해도 괜찮지 않을까해서요

View에서 들고 있고 스크롤뷰로 등록하는게 좀 더 안전하다 생각해서 내부에 들고 있었는데 수정하겠습니다.

[18901d0] 수정했습니다

뷰에서 들고있고 등록하는게 좀 더 안전하다는 게 어떤 의미일까요?

View 내부에서 recipeimageUrls 데이터를 들고 ScrollView에 등록하는게 좀 더 안전하다고 생각했었습니다!

저는 굳이 캐싱할 이유가 안느껴져서 어떤 케이스에서 안정적이지 않은 부분이 발생해서 조치하신건지 궁금했었던거였어요!
일단 수정해주신 부분은 확인했습니다.

@GeonH0 UI 단에서는 데이터 캐싱을 최대한 하지 않고 뷰를 멍청하게 만드는 것이 중요해요.
캐싱할 일이 있다면 꼭 필요한지 고민해주시면 좋을 것 같아요~
왜 뷰가 멍청해야하는지는 찾아봐주시면 재밌을거에요! ㅎㅎ

f-lab-barry · 2024-07-03T14:41:10Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+    private func setupScrollView() {
+        let imageViewWidth = UIScreen.main.bounds.width
+
+        for (index, url) in recipeimageUrls.enumerated() {


전체를 다 돌리는 거라면 for - in 보다 recipeimageUrls.enumerated().forEach를 쓰는 것에 대해 어떻게 생각하시나요?
개인적으로 앞부붙에 반복문을 돌리는 프로퍼티가 보여서 가독성이 좋다고 생각하는데 건호님 생각도 궁금하네요

가독성 측면에서 forEach가 훨씬 좋아보이네요 수정하겠습니다!

[75ce3f8] 수정했습니다

f-lab-barry · 2024-07-03T14:42:04Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+
+            let xPos = CGFloat(index) * imageViewWidth
+            imageView.frame = CGRect(x: xPos, y: 0, width: imageViewWidth, height: 200)
+            scrollView.addSubview(imageView)


이렇게 스크롤뷰를 셋업할 때마다 이미지뷰를 추가해주는 거라면
setupScrollView() 가 의도치 않게 두번 호출되면 어떻게 되는걸까요?

그 부분을 생각 못한거 같습니다. 이미지뷰가 추가 되었는지 플래그를 사용하여 방지하게 수정하도록 하겠습니다

[efc1c64] 수정했습니다

두번째 호출 된 것이 더 올바른 이미지일 수 있지 않을까요?
첫 진입을 막는 것보다 몇번을 호출되더라도 안정적으로 최종적으로 등록한 이미지가 보이도록
기존에 등록한 이미지를 제거한 후(removeFromSuperView) addSubview를 다시 호출해줘야할 것 같다는 의견이었어요.

기존에 등록한 이미지를 제거후 다시 호출하는 메서드로 변경시키겠습니다

확인했습니다~

f-lab-barry · 2024-07-03T14:43:01Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+    }
+
+    private func setupScrollView() {
+        let imageViewWidth = UIScreen.main.bounds.width


화면을 꽉 채우고 싶으신 것 같은데 UIScreen.main에 접근하는 방법 말고는 없을까요?

self.bounds.width 를 사용하는 방법도 있을거 같아요!

f-lab-barry · 2024-07-03T14:43:33Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+            let imageView = UIImageView()
+            imageView.kf.setImage(with: url)
+            imageView.contentMode = .scaleAspectFill
+            imageView.clipsToBounds = true


clipsToBounds는 왜 true로 바꾸신 거에요? (필요한 지점을 찾지 못해서 질문해요)

이전에 이미지 화면에서 양 옆 공백을 만들었다가 지우면서 제거 하지 못한 코드 같아요 제거하겠습니다!

[4830cf5] 제거완료했습니다

f-lab-barry · 2024-07-03T14:45:04Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+        NSLayoutConstraint.activate([
+            scrollView.topAnchor.constraint(equalTo: safeAreaLayoutGuide.topAnchor),
+            scrollView.leadingAnchor.constraint(equalTo: leadingAnchor),
+            scrollView.trailingAnchor.constraint(equalTo: trailingAnchor),
+            scrollView.heightAnchor.constraint(equalToConstant: 200),
+
+            pageControl.topAnchor.constraint(equalTo: scrollView.bottomAnchor, constant: 10),
+            pageControl.centerXAnchor.constraint(equalTo: centerXAnchor),
+
+            photoIndexLabel.topAnchor.constraint(equalTo: pageControl.bottomAnchor, constant: 10),
+            photoIndexLabel.centerXAnchor.constraint(equalTo: centerXAnchor),
+
+            recipenameLabel.topAnchor.constraint(equalTo: photoIndexLabel.bottomAnchor, constant: 20),
+            recipenameLabel.leadingAnchor.constraint(equalTo: leadingAnchor, constant: 20),
+            recipenameLabel.trailingAnchor.constraint(equalTo: trailingAnchor, constant: -20),
+
+            recipedescriptionLabel.topAnchor.constraint(equalTo: recipenameLabel.bottomAnchor, constant: 20),
+            recipedescriptionLabel.leadingAnchor.constraint(equalTo: leadingAnchor, constant: 20),
+            recipedescriptionLabel.trailingAnchor.constraint(equalTo: trailingAnchor, constant: -20)
+        ])


해당 setupUI 메서드가 여러 역할이 있어보이고 상당히 뚱뚱해진 것 같아요. 이부분은 레이아웃을 정의하는 메서드로 분리하는 등으로 개선해보면 어떨까요? (다른 방법으로 분리해주셔도 좋아요)

layout,labels,scroolview,pagecontrol 등으로 분리해보겠습니다!

[0832b39] 수정했습니다

메서드 분리 관점에 대해 더 고민해보시면 좋을 것 같아요
개인적으로 뷰의 addSubview는 한번에 보이는 것이 뷰의 구조를 파악하는데 이점이 있어보여요
어떤 것을 모으고, 모은 것 가운데 어떤걸 메서드로 추출해 분리하는 것이 좋을지 고민해주세요! 😄

넵 좀 더 고민하고 수정해 보겠습니다

f-lab-barry · 2024-07-03T14:46:17Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailView.swift

+
+extension RecipeDetailView: UIScrollViewDelegate {
+    func scrollViewDidScroll(_ scrollView: UIScrollView) {
+        let pageIndex = round(scrollView.contentOffset.x / UIScreen.main.bounds.width)


반올림하면 페이지 수 계산에 문제 없나요?

이미지를 반 이상 넘기면 바로바로 변하기 위해 넣은 건데 페이지 수 계산에는 문제가 없는데 제거 하는게 좋을까요?

의도하신거라면 괜찮습니다!

f-lab-barry · 2024-07-03T14:48:08Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+
+final class RecipeDetailViewController: UIViewController {
+
+    private let recipeDetailView = RecipeDetailView()


Suggested change

private let recipeDetailView = RecipeDetailView()

private let contentView = RecipeDetailView()

뷰컨에서는 contentView로 들고있는게 좋아보여요

[60fa4e2] 수정했습니다

f-lab-barry · 2024-07-03T14:48:19Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+import UIKit
+import RxSwift


써드파티 사이의 개행 부탁드려요

[61a0df9] 수정했습니다

f-lab-barry · 2024-07-03T14:48:45Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+    private let recipeDetailView = RecipeDetailView()
+    private let customNavigationBar = CustomNavigationBar()
+    private let interactor: RecipeDetailInteractor
+    private let disposeBag = DisposeBag()


disposeBag을 let으로 들고있어도 문제 없나요?

disposeBag이 일회용 구독이라 괜찮을거 같아요

취소는 필요하지 않나보군요 넵!

f-lab-barry · 2024-07-03T14:50:00Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+
+    private func setupUI() {
+        view.backgroundColor = .white
+        view.addSubview(recipeDetailView)


addSubview 보다 view = contentView를 해주는게 더 낫지 않을까요?

[690680d] 수정했습니다

f-lab-barry · 2024-07-03T14:50:16Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+    private func setupUI() {
+        view.backgroundColor = .white
+        view.addSubview(recipeDetailView)
+        view.addSubview(customNavigationBar)


상단 네비바를 add 해주는 방향인가요?

네 상단 네비바를 add 해서 사용하려고 했습니다

[690680d] 상단 네비바를 View로 이동시키고 행동만 ViewController에 정의했습니다

f-lab-barry · 2024-07-03T14:50:53Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+}
+
+// MARK: - RecipeDetailInteractorDelegate
+extension RecipeDetailViewController: RecipeDetailInteractorDelegate {


mark 주석 아래 한 줄 개행 해주세요~

[beca64c] 수정했습니다

f-lab-barry · 2024-07-03T14:51:33Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+    func fetchedRecipe(result: Result<Recipe, Error>) {
+        switch result {
+        case .success(let recipe):
+            let recipeItemViewModel = RecipeListMapper().mapToRecipeDetailViewModel(from: recipe)


mapper를 여기서 바로 생성해서 쓰면 추후에 사용할 일이 또 생겼을 때 매번 생성하는건가요?

재 사용성 측면을 전혀 고려하지 않은거 같아요 mapper 인스턴스를 선언후 재사용하도록 수정하겠습니다

[1c0bb26] 수정했습니다

f-lab-barry · 2024-07-03T14:51:50Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+            DispatchQueue.main.async {
+                self.recipeDetailView.configure(with: recipeItemViewModel)
+            }


여기서 비동기가 필요한 이유가 뭘까요?

configure를 통해서 UI 를 업데이트 시키기 때문에 필요하다고 판단했습니다

없는것과 있는 것 쓰레드 테스트 부탁드려요.

f-lab-barry · 2024-07-03T14:52:11Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

+            DispatchQueue.main.async {
+                self.displayError(error)
+            }


여기도요

이 부분은 제거 하겠습니다!

[38f28b5] 제거했습니다

f-lab-barry · 2024-07-03T14:53:28Z

HomeCafeRecipes/HomeCafeRecipes/Resources/Fonts.swift

@@ -10,4 +10,6 @@ import UIKit
 struct Fonts {
    static let titleFont: UIFont = .systemFont(ofSize: 16, weight: .bold)
    static let bodyFont: UIFont = .systemFont(ofSize: 14, weight: .regular)
+    static let DetailtitleFont: UIFont = .systemFont(ofSize: 24, weight: .bold)


Suggested change

static let DetailtitleFont: UIFont = .systemFont(ofSize: 24, weight: .bold)

static let DetailTitleFont: UIFont = .systemFont(ofSize: 24, weight: .bold)

[e3b3ad9] 변경했습니다

맨 앞 대문자도 변경해주세요. 아래 디스크립션 폰트도 대문자로 되어있네요

[f8b8c75]적용했습니다

…abels로 분리

…ionBar를 RecipeDetailView로 이동

f-lab-barry · 2024-07-09T07:47:50Z

HomeCafeRecipes/HomeCafeRecipes/Presentation/Feed/View/RecipeDetailViewController.swift

-            DispatchQueue.main.async {
-                self.displayError(error)
-            }
+                self.displayError(error)            


요거 앞에 인덴트가 많이 들어갔어요~

[a82aabd] 수정했습니다!

f-lab-barry

인덴트만 챙겨주세요. 전체적으로 확인은 했어요

sonarqubecloud · 2024-07-11T14:04:32Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

GeonH0 added 7 commits July 2, 2024 23:27

Feat: RecipeDetailViewModel 정의

efaab96

Feat: RecipeDetailView 정의

27b024e

Feat: RecipeDetailViewController 정의

c566617

Fix: mapToRecipeItemViewModel에서 mapToRecipeDetailViewModel로 수정

6f92e1a

Feat: CustomNavigationBar 정의

80b6640

Feat: DetailView에 들어갈 Fonts 정의

2821348

Fix: DeatilView에 들어갈 Fonts 적용

1f1c396

f-lab-barry reviewed Jul 3, 2024

View reviewed changes

GeonH0 added 17 commits July 4, 2024 22:35

Fix: ViewModel 네이밍 변경

9881443

Fix: 변경된 네이밍 적용

fa15e8f

Fix: 써드파티와 퍼스트파티 사이 한줄 띄우기 적용

d508f7e

Fix: recipeNameLabel로 네이밍 수정

092afac

Fix: recipeDescriptionLabel로 네이밍 수정

1b83f41

Fix: recipeImageUrls로 네이밍 수정

a78dd53

Fix: recipeimageUrls를 스크롤뷰를 구성하는데 바로 전달해서 사용

18901d0

Fix: for - in 반복문에서 forEach로 변경

75ce3f8

Fix: imagesAdded를 사용하여 이미지뷰 중복 추가 방지

efc1c64

Fix: clipsToBounds 제거

4830cf5

Fix: setupUI 메서드를 setupLayout,setupScrollView,setupPageControl,setupL…

0832b39

…abels로 분리

Fix: contentView로 네이밍 변경

60fa4e2

Fix: 써드파티 사이 개행 추가

61a0df9

Fix: RecipeDetailViewController에 view = contentView 처리, customNavigat…

690680d

…ionBar를 RecipeDetailView로 이동

Fix: mark 주석 한 줄 개행

beca64c

Fix: mapper 인스턴스 생성 및 적용

1c0bb26

Fix: DetailTitleFont로 네이밍 수정

e3b3ad9

f-lab-barry reviewed Jul 9, 2024

View reviewed changes

f-lab-barry approved these changes Jul 9, 2024

View reviewed changes

GeonH0 added 4 commits July 9, 2024 17:10

Fix: 불필요한 비동기처리 제거

a82aabd

Add: project.pbxproj 추가

ca4c88a

Fix: 카멜케이스 적용 및 사용하지 않는 ViewModel 제거

14ecdf6

Fix: 기존 등록한 이미지 제거후 다시 addSubview 하는 로직으로 변경

557a785

GeonH0 force-pushed the feature/FeedDetailView branch from b89b6bf to 557a785 Compare July 9, 2024 08:11

Merge branch 'main' into feature/FeedDetailView

4e56909

GeonH0 merged commit af2a046 into main Jul 11, 2024
2 checks passed

GeonH0 deleted the feature/FeedDetailView branch September 12, 2024 01:19

	private let recipenameLabel = UILabel()
	private let recipeNameLabel = UILabel()

	private let recipedescriptionLabel = UILabel()
	private let recipeDescriptionLabel = UILabel()

	private var recipeimageUrls: [URL] = []
	private var recipeImageUrls: [URL] = []


		final class RecipeDetailViewController: UIViewController {

		private let recipeDetailView = RecipeDetailView()

	private let recipeDetailView = RecipeDetailView()
	private let contentView = RecipeDetailView()

	static let DetailtitleFont: UIFont = .systemFont(ofSize: 24, weight: .bold)
	static let DetailTitleFont: UIFont = .systemFont(ofSize: 24, weight: .bold)

레시피 상세화면 UI를 정의해 보았습니다. #12

레시피 상세화면 UI를 정의해 보았습니다. #12

Conversation

GeonH0 commented Jul 3, 2024

Choose a reason for hiding this comment

GeonH0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-barry Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-barry Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeonH0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeonH0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeonH0 Jul 4, 2024 •

edited

Loading

f-lab-barry Jul 5, 2024 •

edited

Loading

f-lab-barry Jul 5, 2024 •

edited

Loading

GeonH0 Jul 4, 2024 •

edited

Loading

GeonH0 Jul 4, 2024 •

edited

Loading