Feat [#43] 마이페이지 뷰 SegmentControl 상단 고정 기능 구현 완료 #49
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -682,7 +687,6 @@ | |||
| 2A8D70C52B4D8079009F4C6C /* UIViewController+.swift in Sources */, | |||
| 2A6D54C12B479B4300F9891E /* adjusted+.swift in Sources */, | |||
| 2A8D70D12B4DD356009F4C6C /* JoinAgreementViewController.swift in Sources */, | |||
| 3C49936B2B4FF118002A99CF /* MyPageSegmentedControlView.swift in Sources */, | |||
| 3C2F54522B51224500E7BF01 /* MyPageAccountInfoViewController.swift in Sources */, | |||
| 3CE9C1352B4C4BC20086E4A3 /* CircleProgressbar.swift in Sources */, | |||
| 3C6193192B3A7AC700220CEB /* Config.swift in Sources */, | |||
There was a problem hiding this comment.
제시된 코드 패치의 주요 변경 사항은 다음과 같습니다:
[...] MyPageSegmentedControlView.swift in Sources와 관련된 부분이 변경되었습니다.- 이 파일에 대한 참조(
3C49936A2B4FF118002A99CF)가 제거되었습니다. PBXBuildFile및PBXFileReference에서 해당 파일을 참조하고 있던 항목들이 삭제되었습니다.
- 이 파일에 대한 참조(
추가적으로, Cells 그룹에 대한 새로운 PBXGroup이 추가되었습니다. 하지만 현재로서는 비어 있습니다.
개선 제안 및 버그 위험 사항 등에 대해서는 보여진 코드만으로는 파악하기 어렵습니다. 전체 소스 코드나 실행 환경 정보를 알 수 없기 때문입니다. 이러한 세부 정보를 함께 제공해주시면 추가적인 도움을 드릴 수 있습니다.
|
|
||
| let shapeLayer = CAShapeLayer() | ||
|
|
||
| shapeLayer.path = bezierPath.cgPath | ||
| shapeLayer.lineCap = .square | ||
| shapeLayer.lineCap = .round | ||
|
|
||
| shapeLayer.strokeColor = circleTintColor.cgColor | ||
| shapeLayer.fillColor = UIColor.clear.cgColor |
There was a problem hiding this comment.
이 코드 패치에 대해 간단한 코드 리뷰를 도와드리겠습니다.
circleTintColor변수의 초기값을.clear로 변경하여 원의 틴트 색상을 투명하게 설정할 수 있게 되었습니다.draw(_ rect: CGRect)함수에서 원 아크를 그리기 위한 베지어 패스 작성시,lineWidth값을 조정하여 좀 더 정확한 그림을 그릴 수 있도록 되었습니다.draw(_ rect: CGRect)함수내에서 subviews와 sublayers를 삭제하는 부분(self.subviews.forEach { $0.removeFromSuperview() }와self.layer.sublayers?.forEach { $0.removeFromSuperlayer() })이 추가되었습니다. 이를 통해 이전에 그려진 원과 진행바를 지우고 다시 그릴 수 있게 되었습니다.- 원모양 진행바(
shapeLayer)의lineCap속성이.square에서.round으로 변경되었습니다. 이로 인해 진행바가 시작과 끝 지점에서 둥글게 그려집니다.
개선사항:
- 코드는 큰 문제가 없어 보이지만,
CircleProgressbar클래스의 일부 속성들 (lineWidth,circleBackgroundColor,circleTintColor)에 대한 값의 범위(유효한 값)나 검증(예외 처리)이 필요한지 확인할 필요가 있습니다. value가 nil인 경우에도 진행바를 그릴 수 있는 처리나 디폴트 값 설정 등이 추가될 수 있습니다.
위의 몇 가지 사항을 확인하여 속성 값의 유효성을 보장하고, 명확한 동작을 보장할 수 있도록 수정하는 것이 좋습니다.
| } | ||
| */ | ||
|
|
||
| } |
There was a problem hiding this comment.
아래는 코드 패치입니다. 코드 리뷰를 짧게 도와드리겠습니다. 버그 위험과/또는 개선 제안이 있는지 확인해주세요.
❌ "import SnapKit" 라인이 @@ -7,23 +7,22 @@ 와 함께 추가되어 코드가 병합된 것으로 보입니다. 따라서 코드 조각 전체가 제공되지 않았으며 누락된 부분을 파악하기 어렵습니다.
❌ setLayout() 함수가 아직 구현되지 않은 것으로 보입니다. 이 함수를 구현하여 뷰의 레이아웃을 설정해야 합니다.
✅ viewDidLoad() 메서드 내에서 setUI()와 setLayout() 함수를 호출하여 UI 요소 설정 및 레이아웃 설정을 분리하는 접근 방식은 좋습니다.
✅ setUI() 함수 내에서 "self.view.backgroundColor = .donPale"로 배경색을 설정하는 것은 올바른 접근입니다.
요약:
- "import SnapKit"을 추가해야 합니다.
- setLayout() 함수를 구현해야 합니다.
- setUI() 함수가 호출될 때 "self.view.backgroundColor = .donPale"와 같이 배경색이 설정됩니다.
| func confirmButtonTapped() { | ||
| transparentButtonPopupView.alpha = 0 | ||
| // ✅ 투명도 주기 버튼 클릭 시 액션 추가 | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰:
-
viewDidLayoutSubviews메서드에서tabBarHeight를 계산하고 있지만, 이 메서드는 레이아웃 갱신시마다 호출되므로 많은 메모리 할당을 유발할 수 있습니다. 대신viewWillAppear등의 메서드에서 한 번만 계산하면 됩니다. -
getAPI()메서드가 비어있어서 기능을 파악하기 힘듭니다. 해당 메서드에 적절한 네트워크 요청 코드나 관련 작업을 추가해야 합니다. -
view.backgroundColor와navigationController?.navigationBar.isHidden에서 사용되는 UIColor 확장을 제거해도 됩니다. "UIColor.donGray1"와 같은 값 자체를 사용해도 괜찮습니다. -
homeCollectionView의snp.makeConstraints부분에서 safeAreaLayoutGuide 대신 self.view를 사용하는 것이 좋습니다. -
setDelegate메서드는 데이터 소스 설정을 누락하고 있습니다.homeCollectionView.dataSource = self를 추가해야 합니다. -
showToast(_:notification:)메서드가 복잡하게 작성되어 있습니다. 애니메이션과 딜레이 로직을 조금 더 개선하고, GCD(dispatch)를 사용하여 적절한 스레드에서 실행되도록 수정할 수 있습니다. -
collectionView(_:cellForItemAt:)메서드에서HomeCollectionViewCell.dequeueReusableCell메서드 호출이 여전히 누락되어 있습니다. 실제로 셀을 반환하기 위해 이를 추가해야 합니다. -
scrollViewDidScroll(_:)메서드에는 스크롤 동작과 관련된 추가적인 로직도 필요합니다. 현재는 스크롤이 아래로 되면 스크롤이 가능하도록 처리되고, 위로 갈 때는 스크롤이 불가능하게 처리되고 있습니다. -
DontBePopupDelegate프로토콜에서 "투명도 주기" 버튼 클릭 시 액션 추가가 필요한 부분에 대해 미구현 상태입니다. -
코드 내의 일부 한글 주석이 보이지만, 문제 없이 작동할 수 있습니다. 다만 주변 코드와 통일성을 위해 한국어 주석을 모두 영문 주석으로 변경하는 것을 고려할 수 있습니다.
-
축약 표현을 사용하여 코드를 좀더 읽기 쉽게 만들 수 있습니다.
-
버그나 리스크 요소는 일목요연하게 확인할 수 없습니다. 전체 소스 코드와 해당 코드를 사용하는 다른 장소를 파악해야 완전한 평가 및 리뷰를 할 수 있습니다.
| scrollView.isScrollEnabled = true | ||
| } else { | ||
| rootView.myPageContentViewController.homeCollectionView.isScrollEnabled = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
아래에 주어진 코드 패치를 간단하게 검토해 드리겠습니다. 버그 위험과/또는 개선 제안 사항을 환영합니다:
-
rootView변수는 현재private으로 선언되어 있으며, 다른 클래스 내에서 접근할 수 없습니다. 그러나 해당 변수로setContentOffset,isScrollEnabled및segmentedControl속성을 설정하는 코드가 있습니다. 따라서rootView변수를private에서internal로 변경해야 합니다. -
moveTop메서드에서myPageScrollView의setContentOffset메서드 호출 시,statusBarHeight변수의 정의가 없습니다. 이 변수가 정의되지 않은 상태에서 사용될 가능성이 있으므로,statusBarHeight변수를 적절히 정의해야 합니다. -
68번째 줄의
setDelegate()메서드에서rootView.myPageSegmentedControlView.pageViewController의delegate와dataSource를 설정하고 있는데, 해당 뷰컨트롤러의 변수명이 변경되었습니다. 변수를rootView.pageViewController로 수정해야 합니다. -
scrollViewDidScroll(_:)메서드에서rootView.segmentedControl의 위치를 조정하고 있습니다. 그러나 해당 코드는UICollectionViewDelegate프로토콜이 아닌UIScrollViewDelegate프로토콜에서 구현되어야 합니다.
코드 리뷰를 위한 조언으로는 변수 및 메서드 이름을 의미 있는 이름으로 변경하여 가독성을 향상시키고, 주석을 추가하여 코드의 목적과 동작 방식에 대해 설명하는 것이 좋습니다. 또한, setUI(), setDelegate(), setAddTarget()와 같은 메서드를 파일 상단에 정의하거나 관련된 부분에서 호출하는 것이 구조적으로 더 좋을 수 있습니다.
| @@ -39,7 +39,7 @@ final class MyPageProfileView: UIView { | |||
|
|
|||
| let userIntroduction: UILabel = { | |||
| let label = UILabel() | |||
| label.text = "안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히히히" | |||
| label.setTextWithLineHeight(text: "안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히히히", lineHeight: 20.adjusted) | |||
| label.textColor = .donGray7 | |||
| label.textAlignment = .center | |||
| label.font = .font(.caption2) | |||
There was a problem hiding this comment.
아래는 코드 패치입니다. 기능이나 향상 제안 외에도 버그의 위험성에 대해 간단한 코드 검토를 도와드리겠습니다:
@@ -39,7 +39,7 @@ final class MyPageProfileView: UIView {
let userIntroduction: UILabel = {
let label = UILabel()
- label.text = "안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히히히"
- label.setTextWithLineHeight(text: "안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히안녕하세요반가와요우히히히히", lineHeight: 20.adjusted)
label.textColor = .donGray7
label.textAlignment = .center
label.font = .font(.caption2)
변경 전 라인 39에서 변경 후 라인 39까지의 코드입니다.
개선 사항:
- setTextWithLineHeight 메서드를 사용하여 UILabel의 텍스트에 줄 높이를 설정하는 것은 좋습니다.
버그의 위험성:
코드 자체에는 명확한 버그의 위험이 없어 보입니다.
추가적으로, setTextWithLineHeight 메서드의 구체적인 구현이나 UILabel 확장에 대한 정보가 제공되지 않았기 때문에 더 많은 검토는 제공하기 어렵습니다.
|
|
||
| transparencyInfoView?.snp.makeConstraints { | ||
| $0.edges.equalToSuperview() | ||
| } | ||
|
|
||
| transparencyInfoView?.bringSubviewToFront(myPageContentView) | ||
| transparencyInfoView?.bringSubviewToFront(self) | ||
| transparencyInfoView?.closeButton.addTarget(self, action: #selector(closeButtonTapped), for: .touchUpInside) | ||
| transparencyInfoView?.infoScrollView.delegate = self | ||
| } |
There was a problem hiding this comment.
해당 코드 패치에 대한 간단한 코드 리뷰를 도와드리겠습니다.
-
dataViewControllers프로퍼티를 선언하고 초기화하지 않았습니다. 초기화된[self.myPageContentViewController, self.myPageCommentViewController]배열을 반환하도록 설정해야 합니다. -
myPageScrollView의showsVerticalScrollIndicator속성을false로 설정했습니다. 이렇게 하면 수직 스크롤 바가 표시되지 않습니다. -
myPageProfileView와myPageSegmentedView의 타입이var로 선언되어 있습니다. 이 두 뷰는 변경할 필요가 없으므로let으로 선언하여 수정하지 않도록 해주는 것이 좋습니다. -
segmentedControl은UISegmentedControl대신에 사용자 정의한MyPageSegmentedControl을 사용하도록 설정했습니다. 해당 컨트롤러를 사용할 경우, 해당 클래스에 대한 구현과 관련된 사항을 확인해야 합니다. -
pageViewController를lazy로 선언하고 초기화합니다.dataViewControllers[0]을 현재 표시된 뷰 컨트롤러로 설정하도록 설정했습니다. -
myPageSegmentedControlView를myPageSegmentedView로 변경했습니다. 해당 뷰에segmentedControl및pageViewController.view를 추가하도록 설정했습니다. -
setLayout()메서드에서 뷰의 상단, 하단, 너비 제약 조건을 수정하였습니다. 또한 다른 서브뷰들에 대한 레이아웃 제약 조건을 수정했습니다. -
setDelegate()메서드를 추가하여 델리게이트 설정 로직을 구현할 수 있습니다. 이 메서드에서는 해당되는 뷰나 컨트롤러의 델리게이트를 설정하는 작업을 수행합니다. -
setAddTarget()메서드에서transparencyInfoView를myPageContentView가 아닌 현재 뷰(self)에 추가하도록 수정했습니다. 또한transparencyInfoView의 하위뷰로 보내는 대신 현재 뷰를 앞으로 가져오도록 수정했습니다.
다음은 코드 개선 사항입니다:
-
직접 값이 할당된 각 UI 요소들이 한 곳에 모여 있지 않고 여러 곳에 흩어져 있습니다. 코드의 유지보수성을 강화하기 위해 값이 할당된 UI 요소들을 한 곳으로 모으는 것이 좋습니다.
-
스크롤 뷰와 컨텐츠 뷰의 크기 제약 조건이 하드코딩되어 있습니다. 동적으로 변경될 수 있는 경우 뷰의 크기를 설정하는 데 더 유연성을 추가하는 것이 좋습니다.
-
myPageScrollView의 배경색이donBlack로 설정되어 있지만, 해당 색상이 실제로 정의되어 있는지 확인해야 합니다. -
pageViewController의 초기 뷰 컨트롤러가dataViewControllers[0]으로 상수화되어 있습니다. 이 값이 변경될 필요 없는 경우 해당 값을 상수로 선언하고 사용하는 것이 좋습니다. -
다른 개발자들과 작업하는 경우 코드 스타일 가이드라인을 따르는 것도 좋은 아이디어입니다. 변수 및 메서드 이름, 들여쓰기, 공백 등에 일관된 패턴을 사용하여 코드의 가독성을 향상시킬 수 있습니다.
이외의 버그 리스크나 개선 제안 사항을 파악하기 위해서는 전체 프로젝트 구조와 해당 클래스의 사용 방법과 목적 등에 대한 더 자세한 정보가 필요합니다.
| func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> CGSize { | ||
| return CGSize(width: 343.adjusted, height: 210.adjusted) | ||
| } |
There was a problem hiding this comment.
P1
이렇게 쓰면 dynamic height 작동 안 됩니다 ! 추후 수정 고려해주세요 :>
| scrollView.isScrollEnabled = true | ||
| rootView.myPageContentViewController.homeCollectionView.isScrollEnabled = false | ||
|
|
||
| if yOffset <= -91 { |
| $0.width.equalTo(myPageScrollView.snp.width) | ||
| $0.height.equalTo(1000) | ||
| $0.height.equalTo(2000) |
There was a problem hiding this comment.
dynamic height 지정이 안되있어서 머지후에 삭제하고 수정하겠습니다!!!
| pageViewController.view.snp.makeConstraints { | ||
| $0.top.equalTo(segmentedControl.snp.bottom).offset(2.adjusted) | ||
| $0.leading.trailing.equalToSuperview() | ||
| $0.height.equalTo(2000) |
| @@ -101,13 +159,13 @@ extension MyPageView { | |||
| self.myPageScrollView.isScrollEnabled = false | |||
|
|
|||
| transparencyInfoView = DontBeTransparencyInfoView() | |||
| myPageContentView.addSubview(transparencyInfoView!) | |||
| self.addSubview(transparencyInfoView!) | |||
| @@ -7,23 +7,22 @@ | |||
|
|
|||
| import UIKit | |||
|
|
|||
| import SnapKit | |||
|
|
|||
| class MyPageCommentViewController: UIViewController { | |||
| } | ||
| */ | ||
|
|
||
| } |
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 코드 리뷰를 도와드리겠습니다. 버그 위험과/또는 개선 제안을 환영합니다.
주어진 코드 패치는 SnapKit 라이브러리를 추가로 임포트하고 UI 구성과 레이아웃 설정하는 데 사용되는 메서드가 추가되었습니다.
버그 위험:
- 현재 주어진 코드 패치에는 명확한 버그 위험이 보이지 않습니다.
개선 제안:
setUI()메서드에서self.view.backgroundColor를 변경하는 논리적 단계가 추가되었습니다. 이로 인해 코드의 가독성과 유지 보수성이 향상될 수 있습니다.setLayout()메서드는 아직 구현되지 않았으므로 해당 메서드를 구현하여 레이아웃 설정에 필요한 코드를 추가해야 합니다.
일반적으로 코드 리뷰는 더 많은 상황과 코드 내용을 고려해야하지만, 주어진 정보로는 일부 개선 제안 및 버그 위험을 알려드릴 수 있었습니다.
| scrollView.isScrollEnabled = true | ||
| } else { | ||
| rootView.myPageContentViewController.homeCollectionView.isScrollEnabled = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
아래는 코드 패치에 대한 간단한 코드 리뷰입니다. 버그 위험 요소 및 개선 제안을 환영합니다:
- 현재 페이지 설정 부분은 안전하지 않습니다.
currentPage값을 저장하는 didSet 속성 관찰자에서 UI 업데이트를 수행하기 때문에, 새로운 페이지로 전환될 때 애니메이션이 완료되기 전에 다른 페이지로 빠르게 전환할 수 있습니다. rootView의 타입은MyPageView입니다. 이것은 접근 지정자가private로 선언되어 있으므로 외부에서 액세스할 수 없습니다. 접근 지정자를fileprivate로 변경하거나public으로 변경하여 문제를 해결할 수 있습니다.setDelegate()메서드에서rootView.myPageScrollView.delegate를 설정하지만,scrollViewDidScroll(_:)메서드는UICollectionViewDelegate프로토콜에서 사용되므로 원하는 동작을 수행하지 못할 것입니다.UIScrollViewDelegate프로토콜을 준수하는 메서드가 필요합니다.setAddTarget()메서드에서rootView.segmentedControl의 addTarget() 메서드를 사용하려면 해당 컨트롤을 정의해야 합니다. 현재 코드에서는MyPageView클래스 내에 해당하는 프로퍼티가 없으므로rootView.myPageSegmentedControlView.segmentedControl으로 변경하거나 해당 컨트롤을 정의해야 합니다.moveTop()메서드 내에서 사용되는statusBarHeight변수의 정의가 코드에 포함되어 있지 않습니다. 이 값도 정의되어야 합니다.
추가적으로, 코드의 다른 부분에서 문제점이 있는지 자세히 검토할 필요가 있습니다.
|
|
||
| transparencyInfoView?.snp.makeConstraints { | ||
| $0.edges.equalToSuperview() | ||
| } | ||
|
|
||
| transparencyInfoView?.bringSubviewToFront(myPageContentView) | ||
| transparencyInfoView?.bringSubviewToFront(self) | ||
| transparencyInfoView?.closeButton.addTarget(self, action: #selector(closeButtonTapped), for: .touchUpInside) | ||
| transparencyInfoView?.infoScrollView.delegate = self | ||
| } |
There was a problem hiding this comment.
아래는 주어진 코드 조각에 대한 간략한 코드 리뷰입니다. 버그 위험 사항 및 개선 제안을 포함하고 있습니다:
-
dataViewControllers의 속성을 사용할 때UIViewController배열이 반환되지만, 이 배열은 항상[self.myPageContentViewController, self.myPageCommentViewController]로 고정됩니다. 이렇게 하면 나중에 다른 뷰 컨트롤러가 추가되거나 변경되어야 할 경우 코드를 수정해야 합니다. 동적인 데이터 소스를 사용하는 것이 좋을 수 있습니다. -
myPageScrollView에서.showsVerticalScrollIndicator속성을false로 설정하는 것은 선언된 시점에서 스크롤 뷰에 적용되기 때문에 원하는 효과를 보장하지 않을 수 있습니다. 스크롤 뷰가 완전히 초기화된 후에 해당 속성을 설정하는 것이 좋습니다. -
myPageSegmentedView를 추가하고segmentedControl및pageViewController.view를 그 안에 넣는 것은 구조적으로 합리적일 수 있으나, 해당 요소들에 대한 제약 조건이 제대로 설정되지 않았습니다. 이러한 제약 조건을 확인하고 필요한 경우 수정하세요. -
setDelegate()함수가 비어있습니다. 해당 함수에 필요한 델리게이트 설정이 포함되어야 합니다. -
transparencyInfoView?.bringSubviewToFront(myPageContentView)에서transparencyInfoView의 부모 뷰에 대한 명시적인 참조가 필요합니다. 현재는myPageContentView로 향하는 참조가 없으므로 작동하지 않을 수 있습니다.
이외에도 코드 조각의 다른 부분에 대한 리뷰가 필요하다면 알려주세요.
👻 PULL REQUEST
💻 작업한 내용
💡 참고사항
📸 스크린샷
📟 관련 이슈