-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix [#96] 마이페이지 디테일 수정 #98
Conversation
@@ -85,7 +85,7 @@ extension DontBeTransparencyInfoView { | |||
|
|||
private func setLayout() { | |||
container.snp.makeConstraints { | |||
$0.top.equalToSuperview().inset(67.adjusted) | |||
$0.centerY.equalToSuperview() | |||
$0.leading.trailing.equalToSuperview().inset(18.adjusted) | |||
$0.height.equalTo(392.adjusted) | |||
} |
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.
이 코드 패치는 DontBeTransparencyInfoView
의 확장입니다. setLayout()
함수 내부에서 container
의 레이아웃을 설정하고 있습니다. 이 부분에 대한 리뷰를 해보도록 하겠습니다.
- 기존 코드:
$0.top.equalToSuperview().inset(67.adjusted)
수정 후 코드:$0.centerY.equalToSuperview()
개선 제안:
- 기존 코드에서
top
constraint를centerY
로 변경했습니다. 이러한 변경은 원하는 결과에 따라 달라질 수 있지만, 컨테이너를 수직 중앙 정렬할 수 있습니다. - 이러한 변경에 따른 버그 위험은 크게 없으며, 레이아웃의 시각적인 모습을 보다 원하는대로 조정하게 됩니다.
그 외에는 주석 코드를 포함한 다른 부분에 대한 정보가 없어서 현재 부위의 일부만 확인할 수 있습니다.
let safeAreaHeight = view.safeAreaInsets.bottom | ||
let tabBarHeight: CGFloat = 70.0 | ||
|
||
self.tabBarHeight = tabBarHeight + safeAreaHeight | ||
} | ||
|
||
override func viewWillAppear(_ animated: Bool) { |
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.
이 코드 패치는 HomeViewController
클래스의 viewDidLayoutSubviews()
메서드를 수정한 것입니다.
여러 가지 개선 사항과 버그 위험이 있습니다:
- 코드에 주석을 추가하여 각 섹션의 역할과 목적을 설명해 줄 수 있습니다.
safeAreaInsets
속성을 사용하여 안전 영역의 하단 여백을 가져오는 것은 좋은 접근 방식입니다.- 탭 바의 높이인
tabBarHeight
를 상수로 정의하는 대신viewWillAppear(_:)
메서드에서도 사용될 수 있도록 속성으로 유지할 수 있습니다. self.
접두사를 사용하지 않아도 되는 경우가 있는데, 여기서는self.tabBarHeight
를 사용할 때만 필요합니다.
적절하게 수정된 코드:
final class HomeViewController: UIViewController {
// MARK: - Properties
private var tabBarHeight: CGFloat = 70.0
// MARK: - Lifecycle Methods
override func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()
let safeAreaHeight = view.safeAreaInsets.bottom
tabBarHeight = tabBarHeight + safeAreaHeight
}
override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
// 추가 작업을 수행하거나 초기화를 진행할 수 있습니다.
}
}
주어진 코드 패치는 큰 문제는 없지만 일부 개선 사항이 있습니다. 나머지는 추가 기능 및 전반적인 앱의 목적과 요구사항 등에 맞게 코드를 구성해야 합니다.
guard let footer = homeCollectionView.dequeueReusableSupplementaryView(ofKind: UICollectionView.elementKindSectionFooter, withReuseIdentifier: "HomeCollectionFooterView", for: indexPath) as? HomeCollectionFooterView else { return UICollectionReusableView() } | ||
return footer | ||
} | ||
|
||
func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, referenceSizeForFooterInSection section: Int) -> CGSize { | ||
|
||
return CGSize(width: UIScreen.main.bounds.width, height: 24.adjusted) |
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.
아래는 코드의 수정 사항입니다:
-
cell.commentNumLabel
에 댓글 좋아요 수를 설정하는 부분에서, 올바른 댓글 수를 표시해야 할 것 같습니다. 현재는commentLikedNumber
라는 동일한 값으로 설정되어 있습니다. 아마도commentNumLabel
에는 댓글의 수를 설정해야할 것으로 보입니다. -
cell.likeStackView.snp.remakeConstraints
부분에서$0.height.equalTo(cell.commentStackView)
로 설정되어 있는데, 아마도cell.commentStackView
대신cell.likeStackView
를 사용하실 의도였을 것으로 추측됩니다. 올바른 스택 뷰와의 높이를 설정해주세요. -
cell.commentStackView.isHidden = true
로 댓글 스택 뷰를 숨기는 부분이 추가되었습니다. 이 변경사항이 의도한 대로 동작하는지 확인하세요. -
collectionView(_:didSelectItemAt:)
함수에서let destinationViewController = PostViewController(viewModel: PostViewModel(networkProvider: NetworkService()))
로 새로운 화면으로 이동하는 로직이 주석 처리되고, 대신 NotificationCenter를 사용하여 화면 전환을 처리하는 로직이 추가되었습니다. 이 변경사항이 원하는 방식으로 동작하는지 확인하세요. -
collectionView(_:viewForSupplementaryElementOfKind:at:)
함수가 삭제되었습니다. 이러한 변경을 수용할 필요가 있는지 확인하세요. -
collectionView(_:layout:referenceSizeForFooterInSection:)
함수에서 반환하는 footer의 크기를 설정하는 부분에 대한 변경은 없습니다.
이 외에는 주요한 버그가나 개선 사항을 찾지 못했습니다.
// guard let footer = homeCollectionView.dequeueReusableSupplementaryView(ofKind: UICollectionView.elementKindSectionFooter, withReuseIdentifier: "HomeCollectionFooterView", for: indexPath) as? HomeCollectionFooterView else { return UICollectionReusableView() } | ||
// return footer | ||
// } | ||
|
||
func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, referenceSizeForFooterInSection section: Int) -> CGSize { | ||
|
||
return CGSize(width: UIScreen.main.bounds.width, height: 24.adjusted) |
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.
해당 코드 패치에 대한 간단한 코드 리뷰를 도와드리겠습니다.
pushViewController
라는 이름의 알림(Notification)을 정적 변수로 추가하고 있습니다.showUploadToastView
변수는 사용되지 않으며 제거할 수 있습니다.deleteBottomsheet
변수는DontBeBottomSheetView
인스턴스를 초기화할 때 사용되지만, 이 변수가 더 이상 사용되지 않는다면 제거할 수 있습니다.refreshControl
은 pull-to-refresh 기능을 제공하기 위해 사용되는 것으로 보입니다.collectionView(_:didSelectItemAt:)
에서 선택된 항목의contentId
를 가져오고,pushViewController
라는 이름의 알림을 발송합니다.collectionView(_:layout:sizeForItemAt:)
에서 셀의 크기를 지정하는 로직이 있습니다.- 주석 처리된
collectionView(_:viewForSupplementaryElementOfKind:at:)
메서드는 현재 사용되지 않으므로 삭제할 수 있습니다. collectionView(_:layout:referenceSizeForFooterInSection:)
에서 footer의 크기를 지정하는 로직이 있습니다.
개선 사항:
- 사용되지 않는 코드와 변수는 제거하는 것이 좋습니다.
refreshControl
과 관련된 액션을 추가하여 새로 고침 동작을 구현할 수 있습니다.- 알림 이름
pushViewController
를 정확히 나타낼 수 있는 이름으로 변경하는 것이 좋습니다.
위에서 언급한 개선 사항을 참고하시면 됩니다.
rootView.myPageCommentViewController.homeCollectionView.isScrollEnabled = false | ||
rootView.myPageCommentViewController.homeCollectionView.isUserInteractionEnabled = false | ||
yOffset = -(navigationBarHeight + statusBarHeight) | ||
rootView.segmentedControl.frame.origin.y = yOffset + statusBarHeight + navigationBarHeight | ||
rootView.segmentedControl.snp.remakeConstraints { |
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.
아래는 코드 패치에 대한 간단한 코드 리뷰입니다:
setNotification()
함수를 추가하여 NotificationCenter로부터의 알림을 처리하는 방법이 향상되었습니다.rootView.myPageCommentViewController.commentData
에 데이터를 할당한 후,data.isEmpty
를 확인하고noCommentLabel.isHidden
속성을 설정하여 무단으로 숨기지 않은 것이 좋습니다.pushViewController()
메서드 내에서destinationViewController
인스턴스를 생성할 때, 새로운PostViewModel
인스턴스를 생성하는 대신 기존 viewModel을 재사용하는 것이 좋습니다.UICollectionViewDelegate
확장으로부터isScrollEnabled
와isUserInteractionEnabled
의 중복된 코드를 제거하여 코드를 간결하게 만들 수 있습니다.
개선사항:
- 3번째 항목에서 변경된 제안에 따라
destinationViewController
의 인스턴스 생성 부분 수정 - 4번째 항목에서 언급한 중복 제거
위의 지적을 참고하여 코드를 수정할 수 있습니다.
transparencyInfoView?.infoScrollView.delegate = self | ||
self.transparencyInfoView?.bringSubviewToFront(self) | ||
self.transparencyInfoView?.closeButton.addTarget(self, action: #selector(closeButtonTapped), for: .touchUpInside) | ||
self.transparencyInfoView?.infoScrollView.delegate = self | ||
} | ||
|
||
@objc |
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.
아래는 코드 패치입니다. 코드 리뷰를 간단히 해드리겠습니다. 버그 가능성 및 개선 제안은 환영합니다:
@@ -158,16 +158,20 @@ extension MyPageView {
private func infoButtonTapped() {
self.myPageScrollView.isScrollEnabled = false
- transparencyInfoView = DontBeTransparencyInfoView()
- self.addSubview(transparencyInfoView ?? DontBeTransparencyInfoView())
- self.transparencyInfoView = DontBeTransparencyInfoView()
- transparencyInfoView?.snp.makeConstraints {
-
$0.edges.equalToSuperview()
- if let window = UIApplication.shared.keyWindowInConnectedScenes {
-
}
window.addSubview(transparencyInfoView ?? DontBeTransparencyInfoView())
- self.transparencyInfoView?.snp.makeConstraints {
-
$0.top.leading.trailing.equalToSuperview()
-
$0.height.equalTo(UIScreen.main.bounds.height)
- }
- transparencyInfoView?.bringSubviewToFront(self)
- transparencyInfoView?.closeButton.addTarget(self, action: #selector(closeButtonTapped), for: .touchUpInside)
- transparencyInfoView?.infoScrollView.delegate = self
- self.transparencyInfoView?.bringSubviewToFront(self)
- self.transparencyInfoView?.closeButton.addTarget(self, action: #selector(closeButtonTapped), for: .touchUpInside)
- self.transparencyInfoView?.infoScrollView.delegate = self
}
이 코드의 문제점이나 개선할 사항은 없어보입니다. 다만, 'transparencyInfoView'가 Optional로 선언되어 있으므로 올바르게 처리되었는지 확인이 필요합니다. 또한 UIApplication.shared.keyWindowInConnectedScenes에서 반환된 window가 nil이 아닌지 확인해야 합니다. 이외에는 버그의 위험이나 개선점은 보이지 않습니다.
@@ -136,7 +144,7 @@ extension PostViewController { | |||
} | |||
|
|||
textFieldView.snp.makeConstraints { | |||
$0.bottom.equalTo(tabBarHeight.adjusted) | |||
$0.bottom.equalTo(self.view.safeAreaLayoutGuide).offset(-tabBarHeight) | |||
$0.leading.trailing.equalToSuperview() | |||
$0.height.equalTo(56.adjusted) | |||
} |
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.
아래는 코드 패치에 대한 간단한 코드 리뷰입니다. 버그 위험과/또는 개선 제안이 있으면 알려주세요:
-
두 번째 변경 사항은 상단의
tabBarHeight
라는 속성을 추가하는 것으로, 초기값은 0으로 설정되어 있습니다. 이 변경 사항은 주변 영역을 고려하여 탭 바 높이가 계산될 수 있도록 합니다.- 개선점:
tabBarHeight
를 초기화할 때, 현재 탭 바의 높이를 가져오는 대신에 상수 값인 70.0으로 설정하는 것이 안전합니다.
- 개선점:
-
viewDidLayoutSubviews
메서드에서tabBarHeight
계산 방식이 수정되었습니다.- 개선점: 기존에는
tabBarController?.tabBar.frame.size.height
를 사용했는데, 이 부분은 제대로 작동하지 않을 수 있습니다. 대신에view.safeAreaInsets.bottom
를 가져와 안전한 영역의 하단 여백을 사용하고, 위에서 언급한 상수 값을 추가하여tabBarHeight
를 계산하도록 수정하는 것이 좋습니다.
- 개선점: 기존에는
-
textFieldView.snp.remakeConstraints
주석 처리된 코드 부분은 활성화되어야 합니다.- 개선점: 해당 코드는 텍스트 필드를 제대로 배치하기 위한 제약 조건을 포함하고 있습니다. 주석을 제거하여 해당 코드 블록이 작동되도록 하세요.
-
getAPI()
메서드는 변경될 필요가 없어 보입니다.
모든 코드가 리뷰되었으며 개선 사항은 다음과 같습니다:
- 탭 바 높이를 초기화할 때, 상수 값을 사용하는 것이 좋습니다.
tabBarHeight
계산에 안전한 영역의 하단 여백인safeAreaInsets.bottom
를 사용하여 계산하는 것이 좋습니다.- 밑줄로 처리된 제약 조건 설정 코드를 주석 해제하여 활성화해야 합니다.
let tabBarHeight: CGFloat = 70.0 | ||
|
||
self.tabBarHeight = tabBarHeight + safeAreaHeight | ||
} | ||
} | ||
|
||
// MARK: - Extensions |
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.
해당 코드 패치에 대한 간단한 코드 리뷰를 도와드리겠습니다. 버그 위험과 개선 제안을 환영합니다:
@@ -22,7 +22,7 @@ final class WriteReplyViewController: UIViewController {
}.eraseToAnyPublisher()
var contentId: Int = 0
+var tabBarHeight: CGFloat = 0
// MARK: - UI Components
@@ -61,6 +61,15 @@ final class WriteReplyViewController: UIViewController {
override func viewWillAppear(_ animated: Bool) {
bindViewModel()
}
+
+override func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()
let safeAreaHeight = view.safeAreaInsets.bottom
let tabBarHeight: CGFloat = 70.0
self.tabBarHeight = tabBarHeight + safeAreaHeight
}
}
// MARK: - Extensions
개선 제안:
WriteReplyViewController
클래스의 주석이 누락되었습니다. 클래스의 목적과 역할을 설명하는 주석을 추가하는 것이 좋습니다.contentId
변수가 정의되지만 사용되지 않는 것으로 보입니다. 필요하지 않은 경우 해당 코드를 제거하는 것이 좋습니다.tabBarHeight
변수는var
로 선언되어 있으므로 외부에서도 수정 가능합니다. 만약tabBarHeight
값을 클래스 내에서만 설정하고 사용한다면,let
으로 변경하여 값이 변경되지 않도록 하는 것이 좋습니다.viewDidLayoutSubviews()
메서드에서self.tabBarHeight
값을 설정하고 있는데, 외부에서 직접tabBarHeight
를 사용하지 않는 것으로 보입니다. 이 경우에도let
으로 변경하는 것이 좋습니다.viewWillAppear(_:)
메서드 내부에서bindViewModel()
을 호출하고 있는데, 해당 메서드의 구현은 주어지지 않았습니다.bindViewModel()
메서드가 필요한 경우도 마찬가지로 코드를 제공해주셔야 합니다.
@@ -149,7 +149,7 @@ extension WriteTextView { | |||
keyboardToolbarView.snp.makeConstraints { | |||
$0.leading.trailing.equalToSuperview() | |||
$0.height.equalTo(56.adjusted) | |||
$0.bottom.equalToSuperview() | |||
$0.bottom.equalTo(self.safeAreaLayoutGuide) | |||
} | |||
|
|||
circleProgressBar.snp.makeConstraints { |
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.
제가 어떠한 코드 리뷰를 진행하겠습니다. 주어진 코드 패치의 변경 내용은 keyboardToolbarView.snp.makeConstraints
의 bottom.equalToSuperview()
를 $0.bottom.equalTo(self.safeAreaLayoutGuide)
로 수정하는 것입니다.
이 변경 사항은 안전 영역 레이아웃 가이드에 대한 제약 조건을 사용하도록 수정하는 것으로 보입니다. 이와 같은 변경으로 인해 화면 하단에 위치한 키보드 도구 모음 뷰의 배치가 조정됩니다.
추가적인 버그 위험성이나 개선 제안은 보이지 않습니다. 다만, 제공된 코드 스니펫은 완전하지 않으므로, 전체 코드 컨텍스트를 고려하여 보다 자세한 검토가 필요할 수 있습니다.
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.
수고하셨습니다 !!
👻 PULL REQUEST
💻 작업한 내용
📸 스크린샷
📟 관련 이슈