-
Notifications
You must be signed in to change notification settings - Fork 115
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
오픈 마켓 [STEP 2] 애플사이다, 허황 #96
Conversation
- ViewController의 setupGridFlowLayout, setupCollectionView, registerCell 메서드 추가
- cellForItemAt에서 상품 데이터 반영
- changePriceAnddiscountedPriceLabel, setupThumbnailView, setupSubviewsOfVerticalStackView 메서드 추가 - UICollectionView 익스텐션 추가 - Int 익스텐션 추가
- CALayer 익스텐션 추가
- ProductCellProtocol 추가
- ProductCellProtocol 메서드 수정 - ProductCellProtocol 채택한 뷰 내부 수정
- Cell 형식별 Cell Size, LineSpacing 등을 delegate 메서드의 반환값을 통해 변경 - Custom Cell의 접근제어자 수정 및 네이밍 개선
- Navigation Bar에 Add 버튼 추가, 화면 전환 구성
- decode result switch문으로 처리
- currentScrollRatio 메서드 추가 - Mark 주석 추가
- List 및 Grid 형태의 Layout을 구분함
- UIImageView Extension 메서드 추가 - loadImage() NSCache를 사용해서 메모리에 이미지가 있을 경우 메모리에서 이미지를 불러오고 아니라면 URL로 이미지를 생성함.
- Product 타입의 price, bargainPrice, discountedPrice 프로퍼티 타입 변경 (Int -> Double) - Double 타입 익스텐션 추가
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.
이번 스텝도 수고하셨습니다. 👏
컬렉션뷰가 쉽지 않죠... 저도 아직도 어렵답니다..
코멘트와 함께 질문해주신 부분에 대해 끄적여보았습니다 ✏️
- CellForRowAt
- 현재가 아니라 요구사항이 변경되어서 셀의 구성요소가 많아지고 넘겨줘야하는 데이터가 많아질 경우를 생각해보면 어떤가요?
- 이 부분에 대해서는 prepareForReuse는 말그대로 셀을 세아용하기 위해 clean up 해주는 메서드이지 레이아웃을 하는 메서드는 아니라고 알고 있어요. Self-sizing을 하지 않는다면 cellForItemAt의 메서드가 정말로 레이아웃을 하는 곳인지, 컬렉션뷰에서의 셀 레이아웃이 이뤄져야하는 곳은 어딘지 학습하면 될듯합니다.
-
Cell의 UIImage에 이미지 데이터를 반영
제가 이 질문에 대해 완벽히 이해하지 못했습니다ㅠㅠ
if문을 활용하지 않아도된다는 뜻일까요?
다만, 이미지크기가 클지 안클지는 클라이언트에서 정하는 것이 아니기 때문에 저는 이미지크기를 고려하지 않는 코드가 더 좋다고 생각합니다. 실제 앱을 사용해보니 느려서 이미지 크기에 따라 코드를 매번 바꿔야하는 것이 더 좋을까요? -
공식문서에 명확하게 나와있지 않아서 정확하진 않지만 제가 이해하고 있는 것으론 performBatchUpdates의 updates 클로져의 코드들이 순서대로 진행이 되는게 아니라 그 코드블럭을 넘김으로 인해서 그 변경사항이 반영되는 것으로 알고 있습니다. 그렇기에 해당 메서드 코드를 만나면 바로 updates블록을 실행하되 실질적으로 updates의 코드가 읽힐 때 클로져가 UI에 바로 반영되는 것은 아닌거죠(내부적으로 이런것들을 할거야!를 넘기겠죠 ) 이후 비동기로 바로 메서드 다음 코드들로 넘어가게 될거고, updates 클로져로 넘겼던 operations들이 끝나면 completion으로 넘어오겠죠
-
코멘트에 남겼습니다. 코멘트에 메서드를 분리하지 않는 방법으로는 여기서 ProductPage라는 API를 보내줬따면 실질적으로 뷰컨트롤러가 직접 decoding을 하지 않고 뷰컨트롤러는 정말 결과로써의 필요한 products 데이터 또는 에러만 받도록 하는게 뷰컨트롤러의 역할을 줄일 수 있는 방법이라고 생각합니다.
-
코드적인 레벨을 말씀하시는건지 이런 경우에 UI적으로 어떻게 처리할 수 있는지 둘 중에 어느것을 말씀하시는걸까요? 현재 나오는 UI상의 버그같이 보이는 부분은 performbatchUpdate나 reloadData의 꼬임 문제로 보이네요. performBatchUpdates에 대한 메서드의 사용법이 잘못된거같습니다. 위의 3번에서 나온대로라면 현재 completion에서의 reloadData가 적절할까요?
-
괜찮다고 생각합니다. layer가 아니라 항상 위에 떠있는 뷰로 hidden 처리 등을 해줘도 상관없겠죠(뷰는 정말 다양한 방법들이 많습니다.)
-
이 부분도 이해를 하지 못했습니다. 추가설명을 해주실 수 있나요?
어디에서 estimatedItemSize를 변경하고 어디에서 값을 확인했는데 변경되지 않았다는 것일까요?
import UIKit | ||
|
||
class AddProductViewController: UIViewController { | ||
var titleLabel: UILabel! |
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.
내부 속성을 감추기 위해서 private을 고려해보고, 위험한 강제언래핑을 사용하지 않도록 하는건 어떨까요
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.
private let titleLabel: UILabel = {
let label = UILabel()
label.text = "상품 등록"
label.textAlignment = .center
label.font = .preferredFont(forTextStyle: .title1)
return label
}()
저장 프로퍼티에 클로저를 담는 방법으로 개선했습니다:]
|
||
private func setupTitleLabel() { | ||
titleLabel = UILabel() | ||
titleLabel.text = "상품 등록" |
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.
상품 등록 View는 현재 STEP에서는 요구사항을 확인할 수 없어서 임의로 작성해봤습니다. 다음주에 말씀해주신 부분을 고려해서 구현해보겠습니다. :)
|
||
import UIKit | ||
|
||
class OpenMarketViewController: UIViewController { |
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.
'final' 키워드라는 것을 한번 찾아보는것도 좋아요 !
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.
상속할 필요가 없는 클래스에 붙이는 키워드로 알고 있습니다:)
프로젝트 내 상속할 필요가 없는 클래스에 final 키워드를 붙였습니다😊
} | ||
} | ||
|
||
var currentLayoutKind: LayoutKind = .list |
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.
OpenMarketViewController 내부에서만 사용하는 프로퍼티 및 메서드에 모두 접근제어자를 추가했습니다. :)
view.backgroundColor = .white | ||
} | ||
|
||
private func fetchProductData() { |
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.
fetchProductData라는 메서드를 재사용하기 위해서
api를 파라미터로 받거나 pageNumber, itemsPerPage 등을 파라미터로 받을 수 있겠네용!
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.
fetchData<T: Codable>(api: APIProtocol,
decodingType: T.Type,
completionHandler: @escaping ((_ data: T) -> Void))
메서드를 NetworkDataTransfer 타입에 인스턴스 메서드로 추가해줬습니다:)
fatalError() | ||
} | ||
|
||
// DispatchQueue.global().async { |
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.
기능을 추가하면서 깜빡하고 지우지 못했습니다.
꼼꼼하게 코드를 작성하는 습관을 들이겠습니다😭
} | ||
|
||
private func setupViewController() { | ||
view.backgroundColor = .white |
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.
내부 속성에 접근할 때 어떤 기준으로 self
를 붙이고 계신가요
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.
클로저 캡쳐리스트를 사용하거나, 함수의 매개변수 명과 프로퍼티 명이 같아 혼란을 줄 수 있을 때 self 키워드
를 사용했습니다.
OpenMarketViewController.swift 148번 라인에 위와 같은 경우가 아닌데 사용한 self 키워드
는 삭제했습니다.
import UIKit | ||
|
||
extension String { | ||
func strikeThrough() -> NSAttributedString { |
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.
extension을 사용해서 문자열에 기능을 사용하는것과 파라미터로 문자열을 받아서 기능을 입혀서 output을 뽑느것에는 어떤 차이가 있을까요
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.
별도의 타입을 만들어서 인스턴스 메서드로 만들게 되면 메서드를 사용할 때마다 타입의 인스턴스를 생성해줘야 합니다.
위와 다르게, extension을 사용하여 메서드를 추가해주면 모든 String 타입의 인스턴스들에게 추가해준 메서드가 노출되게 됩니다.
여러가지 방법을 생각해봤습니다.
-
Protocol + CustomLabel
현재 취소선은 Cell의 priceLabel만 사용하는 기능이기 때문에 해당 Label에서만 메서드가 노출될 수 있도록 프로토콜과 익스텐션을 사용하는 방법
Strikable 프로토콜을 만들고 UILabel을 상속해준 뒤 extension을 활용하여 메서드를 기본 구현했습니다.
Strikable 프로토콜 채택한 StriableUILabel 타입을 만들어 Cell의 priceLabel의 타입으로 지정해줬습니다. -
UILabel Extension
기존 String을 Extension하게되면 AttributedText와 관련 없는 String 인스턴스에 메서드가 노출되기 때문에 UILabel로 범위를 좁혀 확장하는 방법
1번 방법으로 처음 개선하려 했으나 2번 방법으로 진행하게 되면 Protocol이나 CustomLabel를 따로 구현하지 않아도 되기 때문에 2번 방법을 사용해서 개선했습니다.
import UIKit | ||
|
||
extension UIImageView { | ||
func loadImage(of key: String) { |
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.
모든 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.
현재 STEP에서 필요한 모든 ImageView
가 서버에서 받아온 이미지이므로 해당 메서드를 사용해야 해서 익스텐션이 적절하다고 판단했습니다. 다음 STEP에서 다른 종류의 이미지를 사용하게 되면 그때 반영해보도록 하겠습니다. :)
@@ -0,0 +1,29 @@ | |||
import UIKit | |||
|
|||
class ViewTypeSegmentedControl: UISegmentedControl { |
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.
ViewType네이밍으로만 어떤 뷰의 타입에 대한 것인지 모호함이 있을 수 있을거 같아요
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.
List/Grid View 종류를 나타내는 LayoutKind
열거형의 네이밍을 사용하여 LayoutKindSegmentedControl
로 수정했습니다.
- Double+Extension 파일 생성
- NetworkDataTransfer 인스턴스 메서드로 이동 - 매개변수로 api, 변환할 모델 타입, completionHandler 받음
메이슨 강같은 피드백 감사합니다. 👍
->
-> reloadData 메서드를 completion 클로저에 배치한 것은 Cell의 Layout이 변경되고 난 이후에 실행하기 위함이었습니다. (FlowLayout Delegate 메서드가 실행된 이후 completion 클로저가 실행되기 때문입니다.)
-> 메서드를 구현하던 당시에는 estimatedItemSize가 변경되지 않았다고 생각했는데, 다시 시도해보니 변경되는 것을 확인했습니다. 😅 중간중간 방향을 잘 잡아주셔서 STEP2 무사히 마칠 수 있었어요. 감사합니다. 👍 |
메이슨 안녕하세요. @myssun0325
애플사이다 @just1103, 허황 @hwangjeha 입니다.
STEP2 완료하여 PR 드립니다. View를 인터페이스 빌더를 사용하지 않고 코드로만 구현해봤는데, 하다보니 ViewController가 너무 커졌네요. 😅 이번 리뷰도 잘 부탁드립니다. :)
STEP2 구현내용
OpenMarket
,
를 넣어주는 메서드 확장고민한 점
1. 리스트, 그리드 형태를 스위치 할 수 있는 방법
리스트와 그리드 형태로 뷰를 구성해야하는데 여러가지 방법을 찾아봤습니다.
4가지 방법을 고민했고, 메이슨께서 필드에서는 3,4번 방법을 주로 사용한다는 답변을 주셔서 저희 모둠도 3번 방식으로 진행했습니다.
결과적으로
1개의 FlowLayout
을 구현하고, Segmented Control에 따라 FlowLayout Delegate를 통해 Layout을 변경하도록 했고,List 및 Grid 형태에 대한
두 가지 Custom Cell
을 구현했습니다.2. Cell Size에 따른 FlowLayout 설정 방법
처음에는 STEP2를 진행할 때 List Cell, Grid Cell 두 가지 모양을 대응할 수 있는
FlowLayout
두 개를 만들고 세그먼트 컨트롤에 따라 collectionView의 레이아웃을 변경하는 1번 방법으로 구현했습니다. 나중에 리팩터링하면서 코드가 중복된다는 느낌을 받아 UICollectionViewDelegateFlowLayout 델리게이트 메서드를 활용하여 동일한FlowLayout
의 설정값을 Cell마다 다르게 지정해 줬습니다. 이 방법이 적절한지 궁금합니다. (현재 Cell의 UI 요소 크기에 따라서 자동적으로 Flow Layout의 Item 크기가 결정되는 방식이 아니라서 나중에 유지보수가 어렵지 않을까 고민이 되기도 했습니다.)궁금한 점
1. CellForItemAt 메서드 관련
Cell의 Label/ImageView에 parsing한 데이터를 반영하는 기능을 처음에는
CollectionView DataSource
의CellForItemAt
메서드에 구현했습니다. 그런데 해당 부분이 View를 수정하고 있기 때문에 코드가ViewController
보다는Cell
에 위치하는 것이 좋다고 판단하여 Cell의updateLabels/updateThumbnailView
메서드를 추가했습니다. 이렇게 코드를 배치한 것이 적절한지 궁금합니다.또한 재사용 Cell에 대해 초기화를 하는 부분도 보통 Cell의
prepareForReuse
메서드에서 구현하는 것으로 알고 있지만, 마찬가지 이유로CellForItemAt
메서드에서 현재 선택된segmented control
에 따라 Cell의 Layout을 조정하도록 했습니다. 이렇게prepareForReuse
를 활용하지 않아도 괜찮을까요?2. Cell의 UIImage에 이미지 데이터를 반영
cellForItemAt
메서드에서 Cell의 이미지를 반영하는 작업을DispatchQueue 메인 큐
에 비동기로 처리하도록 했습니다. 만약 이미지 크기가 크면 아래와 같이 if문을 활용해서 화면 스크롤 시 이미지 Loading으로 인한 버벅거림을 방지할 수 있다고 알고 있습니다. 하지만 이번 프로젝트의 이미지 데이터는 크기가 작아서 버벅거림이 발생하지 않아서 사용하지 않았는데, 이렇게 처리를 하지 않아도 괜찮은지 궁금합니다.3. performBatchUpdates 메서드의 작업처리 방식
collectionView의

performBatchUpdates
메서드는 비동기로 동작하는 메서드일까요?performBatchUpdates
내부에서 View 요소에 접근했는데 동작하지 않아 다른 스레드에서 동작하는 메서드인가 추측했습니다.공식문서를 찾아봐도 비동기라는 설명이 나와있지 않습니다.
아래 그림과 같이
메인 스레드
가 아닌글로벌 스레드
에서 동작하는걸 확인했습니다.4. 이중 switch문 처리
fetchProductData 메서드 내부에서 NetworkDataTransfer로 서버에서 데이터를 받아오는 부분도 반환값이 Result 타입이고, JSONParser로 JSON 데이터를 decoding하는 부분도 반환값이 Result 타입이라서 불가피하게 이중 Swtich문으로 처리를 해야 했습니다. 아래와 같이 처리해도 무방할지, 아니면 decode 메서드의 반환타입을 다시 옵셔널로 수정하는 것이 좋을지 고민이 됩니다.
5. Scroll 위치
List 화면에서 Grid 화면으로 넘어올 때, 사용자가 Scroll한 마지막 위치의 Cell을 확인할 수 있게 최근 위치의 Scroll을 공유하도록 구현하고자 했습니다. 하지만 List 화면에 비해 Grid 화면의 Scroll Indicator가 다소 아래로 내려가 있는 문제가 발생했습니다. 이를 해결하기 위해 List Cell과 Grid Cell의 상대적인 크기를 고려하여 Scroll Offset을 지정하도록 시도해봤지만, 제대로 위치를 잡을 수 없었습니다. 이러한 상황에서 Scroll 위치를 계산할 수 있는 방법이 있을까요?현재 화면의 스크롤 위치의 비율을 계산해서 구현했습니다. 하지만 화면이 전환될 때 아래 gif처럼 스크롤의 잔상이 보이는 문제가 발생했습니다. 개선할 수 있는 방법이 있을까요???
6. Activity Indicator
List 화면과 Grid 화면을 전환할 때도 Activity Indicator를 보여주고 싶었습니다.
하지만 Activity Indicator, collection View 둘 다 메인 뷰컨의 subview로 등록되어 있어 Activity Indicator가 collection View의 영역을 침범해 레이아웃이 깨지는 현상이 발생했습니다.
방법을 고민하던 중 메인 뷰컨에 layer를 하나 추가해주고 추가해준 layer의 subview로 Activity Indicator를 등록하는 방법을 생각해봤지만 아직 구현하지는 못했습니다. 저희가 고민해본 방향으로 진행해도 괜찮을까요???
7. EstimatedItemSize 값 변경
2번 기능을 구현하기 위해 처음에는 Segmented Control에 따라
UICollectionViewFlowLayout
의EstimatedItemSize
값을 변경하고자 했습니다. 그래서 아래와 같이 시도했지만, 코드가 실행되었음에도 estimatedItemSize가 변경되지 않는 것을 확인했습니다. 컴파일 에러가 발생하지 않아서 get-only 프로퍼티도 아닌 것 같은데 이유가 뭘지 궁금합니다.조언을 듣고싶은 점