Skip to content
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] 아리,제리 #93

Merged
merged 34 commits into from
Jan 13, 2022

Conversation

leeari95
Copy link
Member

@leeari95 leeari95 commented Jan 11, 2022

안녕하세요. 그린 @GREENOVER

아리 @leeari95, 제리 @llghdud921 입니다!
STEP2를 구현해서 PR드리는데 제대로 구현한건지 잘 모르겠네요. 뷰 그리는게 젤 어렵습니다...🥲

일단 FlowLayout 관련 부분에서 문제가 발생한 거 같은데, 아무리 찾아봐도 원인도 정확히 알기가 어렵고, 해결도 하지 못했습니다. 이 부분은 같이 한번 봐주셨으면 좋겠습니다. 🙏🏻

상품 등록의 경우 화면 구현까지는 진행하지 않아도 된다고 답변받아서 간단한 네비게이션 아이템만 구성한 후 마무리 하였습니다.

이외에 부족한 점이나 놓친 부분이 있다면 번거롭더라도 한번 더 짚어주시면 감사하겠습니다. 🙇🏻‍♀️

구현 화면

고민했던 점

1. CollectionView 하나로 Cell 두개를 활용하여 화면을 전환하기

  • Custom Cell을 구현하고, 두개의 레이아웃을 만들어 셀만 바꿔주는 방식으로 목록화면 구성
  • FlowLayout을 활용하여 Cell의 레이아웃을 구성
    • SegmentControl을 조작하여 CollectionViewFlowLayout을 변경
  • 서버에서 상품 목록을 불러오는 부분과 뷰를 그리는 부분 비동기 처리 구현

2. CollectionView cell 각각 xib로 구현

  • CollectionViewGridCell, ListCell을 각각 xib파일을 생성하여 storyboard로 구현하였고 두개의 xib에 대한 코드는 ProductCell 하나의 cell로 구성

3. Network를 통해 data를 가져와 CollectionView를 구성

  • API의 Data를 가져오기 위해 productList Search하는 request 생성하여 networkManagerfetch()로 network를 진행하였고 가져온 datacollectionViewload하였습니다.

    let request = networkManager.requestListSearch(page: 1, itemsPerPage: 10) else {
    ...        
    networkManager.fetch(request: request, decodingType: Products.self) { result in
        switch result {
            case .success(let products):
               self.productList = products.pages
               self.collectionViewLoad()
            ...

4. CollectionView를 재구성하는 경우 reloadData() 사용

  • SegmentControl을 이용해 flowlayout을 변경하는 경우 CollectionView를 재구성하기 위해 reloadData를 사용하였습니다.

    // list -> gird, grid -> list로 변경
    @IBAction private func switchSegmentedControl(_ sender: UISegmentedControl) {
            switch sender.selectedSegmentIndex {
            case 0:
                currentCellIdentifier = ProductCell.listIdentifier
                collectionView.setUpListFlowLayout()
                collectionView.scrollToTop()
                collectionView.reloadData()
    

5. alert을 이용한 Error Handling

  • OpenMarket app에서 발생한 error는 alert 창을 띄워 error를 나타내었습니다.
  • localizedError 프로토콜의 errorDescription을 이용하여 description을 정의하였고 error.localizedDescription으로 error message를 출력하도록 에러핸들링 구성.

6. 상품등록 버튼 Segue

  • HIG를 참고하여 상품등록 버튼을 눌렀을 때 Navigation 형태가 아니라 Modal로 띄우도록 구성
  • Navigation Bar를 활용하여 취소 버튼을 구성

궁금한 점 / 조언이 필요한 부분

  • collectionview의 flowlayout을 변경할 때 AutoLayout 충돌 관련 경고가 뜨는데, 해결 방법을 모르겠습니다.
    • 이미 잡혀있는 flowLayout을 앱을 실행한 상태에서 한번 더 대입해서 발생하는 에러인건지.. 잘 판단이 서질 않는데요. 이 부분에 대해서 이런식으로 구현하는게 맞는건지가 의문입니다. 보통 이번 프로젝트처럼 컬렉션뷰로 여러 종류의 셀을 구현할 때, 어떻게 구현하는게 맞는건지 궁금합니다.
  • SegmentControl을 활용하여 List나 Grid를 전환할 때 생기는 약간의 딜레이의 원인을 모르겠습니다.
    • 이 부분도 FlowLayout을 바꾸고 reload해주고 있는데요. reload 부분 때문에 생기는 딜레이 같다고 추측이 됩니다. 현재 flowLayout을 활용해서 화면전환을 하고있는데, 이러한 부분의 설계를 바꾸어야 할까요?
  • 네트워크를 통해 리스트를 불러올 때 현재의 위치가 적절한건지 궁금합니다.
    • 현재 MainViewController에서 네트워크를 통해 상품목록을 프로퍼티인 productList에 담아준 후 dataSource 대리자를 지정해주고 있습니다. 대리자를 지정해주는 순간 셀을 만들기 시작하는데요. 이 부분 적절한 걸까요?
    • 저희의 의도는 네트워킹을 통해 상품목록을 모두 전달받은 후, 셀을 구성할 수 있도록 해주기 위해 이렇게 설계되었습니다.

이 외에 이번 프로젝트 내용은 아니고 STEP1 내용인데, 궁금해서 실제 API를 찌르는 테스트를 진행했다가.. multipart/form-data 부분의 코드가 잘못되었다는 걸 발견하게되어 해당 부분을 전체적으로 리팩토링 하였습니다. 아직 fake 테스트는 배우질 않아서, 실제 API에서 응답받는 방식의 테스트를 통해 검증을 하였습니다.

이번 피드백도 잘부탁드리겠습니다. 그린🍏

leeari95 and others added 24 commits January 7, 2022 10:22
- 코드 수정 후 테스트 진행하다가 네이밍 수정안된 부분 수정
- 테스트의 iOS Deployment Target을 15에서 14로 수정
- listCollectionViewCell 파일과 xib 파일 추가
- ListControllerview 생성 및 관련 속성 구현
- NetworkManger를 생성하여 fetch() 실행
- 네트워크 결과로 productList 가져옴
- productlist를 가지고 collectionview 구성 로직 작성
- 뷰 재구성을 위해 reloadData() 구현
- ProductCell 재사용시를 대비하여 색상, 텍스트, 스타일을 초기화하는 resetLabel 메소드 추가
- priceLabel, discountPriceLabel, stockLabel의 각각 스타일을 설정하는 메소드 추가
- ProductCell의 IBOutlet 네이밍 수정, 필요없는 부분 삭제
- String 타입 extension 추가
- Int 타입 extension 추가
- Product 타입에 포맷팅된 String을 반환하는 메소드 추가
- 네비게이션 바 아이템 우측에 버튼 추가
- 우측 네비게이션 바 아이템 클릭 시 modal 창을 띄우도록 구현
- modal 창을 띄웠을 때 취소 버튼 추가 후 창을 닫을 수 있도록 구현
- ProductCollectionView class 생성
- mainViewController내 collectionview 로직 분리
- UIsrcollview extension 코드 분리
- mainViewController내 불필요한 코드 제거
- 프로퍼티 접근제어 추가
- 메소드 순서 변경
- 0으로 하드코딩된 부분 .zero로 수정
- 이미지를 불러오는 부분 에러처리 추가
- ProductsCollectionView 내 코드 재배치
- mainViewController내 접근제어자 설정
- secretSearch case 추가
- 더이상 사용하지 않는 MultipartFormProtocol 제거
- MultipartForm 내부 API문서 참고하여 전반적으로 수정
- Identifier를 프로퍼티로 추가 후 하드코딩된 부분 개선
- 상품 수정 메소드 반환타입 옵셔널 타입을 논옵셔널로 수정
- requestRegister 메소드 반환타입 수정, requestMultipartForm 메소드 반환타입 수정
- createBody 메소드 내부에서 json 인코딩을 하도록 수정하여 그에따른 반환타입 수정
- 사용하지 않는 jsonMultiPartForm 메소드 제거
- imageMultiPartForm 메소드 내부 form-data 양식을 오류나지 않도록 수정
- Form 타입의 rawValue를 추가
- ProductRegistration 타입 내부 프로퍼티명 수정, Codingkeys 추가
- 수정에 따른 테스트 코드 전반적으로 수정
- reload 메서드명 collectionViewLoad로 변경
- collectionViewLoad() 내부에 collectionview datasource 지정
- reloadData 불필요한 코드 삭제 및 중복 제거
@GREENOVER GREENOVER self-requested a review January 11, 2022 06:58
Copy link
Member

@GREENOVER GREENOVER left a comment

Choose a reason for hiding this comment

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

@leeari95 @llghdud921
아리제리 고생많으셨어요!👍
역시 빠르시군요ㅎㅎ
전체적으로 기능 동작에 전혀 문제가 없긴한것 확인했습니다!
그렇기에 이번에는 리뷰 지옥은 없을거라고.. 생각..해..요..ㅋㅋㅋ


칭찬 드리고 싶은 부분

  • 전체적으로 뷰를 잘 짜셨다고 생각합니다 잘 나눠서 구조적으로 💯
  • 리드미 작성 꾸준한거 너무너무 최고입니다 💯
  • 스텝1에서 이어지는 리팩토링도 잘 되었네요 💯
  • UI가 기획보다 더 예쁘고 애플스럽습니다 💯

고민하셨던 부분에 대한 저의 의견

  1. CollectionView 하나로 Cell 두개를 활용하여 화면을 전환하기
  • 잘 구현된것 같아요!
    다만 그리드/리스트 뷰 전환 시 오토레이아웃 에러가 발생하는데 (앱이 터지진 않지만)
    이것과 관련해서는 리뷰 코멘트로 남겨봤씁니다!
    아마 아래에서 잘 안풀리셨다고 한 부분과 연관이 있을것 같습니다🤔
  1. CollectionView cell 각각 xib로 구현
  • xib로 잘 구현되었고 더할 코멘트도 없이 좋습니다👍
  1. Network를 통해 data를 가져와 CollectionView를 구성
  • 잘 구현됨 확인했습니다!
    리뷰에는 까먹고 못달았는데 외적으로 궁금한 부분이 있습니다.
    requestListSearch(page: 1, itemsPerPage: 10)해당 메서드 호출 시 인자는 고정적으로 저렇게 주는건지 궁금합니다🙋🏻
  1. CollectionView를 재구성하는 경우 reloadData() 사용
  • 현재 최종 코드에서는 reloadData 메서드 호출을 분기에서 각 처리 후 마지막에 두신것 같아요.
    근데 위에서 언급해주셨을때는 각 분기안에 들어가있습니다.
    어떤 차이가 있으며 파생되는 결과가 어떤게 있을까요?🤔
  1. alert을 이용한 Error Handling
  • 얼럿 구현 아주 좋네요ㅎㅎ
    더욱 사용자에게 친절한 앱이 되었어요!
    그런데 얼럿이 나타나도록 테스트를 혹시 해보셨을까요?
  1. 상품등록 버튼 Segue
  • 틀을 잘 잡아 놓으셨네요!ㅎㅎ
    혹시 화면전환에서 Navigation 방식과 Modal 방식을 선택하는 가장 큰 기준점이 어떤게 있을까요?🙋🏻

궁금하거나 조언이 필요한 부분에 대한 저의 의견

  1. collectionview의 flowlayout을 변경할 때 AutoLayout 충돌 관련 경고가 뜨는데, 해결 방법을 모르겠습니다.
  • FlowLayout이나 셀의 구현에는 문제가 없어 보입니다.
    다만 이걸 가져와 뷰에서 사용할때의 파악하신대로 오토레이아웃 관련 문제가 발생합니다.
    이와 관련해 조금 나름 상세하게 리뷰 코멘트를 달아봤습니다!
    (부족하다 싶으시면 꼭 말씀해주세요🥲)
    한번 확인해보심이 좋을것 같습니다.
    더불어 디버깅창에 나오는 에러 메시지를 가지고 디버그해보거나 구글링해보는 습관을 들여보면 좋을것 같습니다!🙌
    왠만한 에러에 관해서는 디버그 메시지가 친절히 알려줍니다~!
  1. SegmentControl을 활용하여 List나 Grid를 전환할 때 생기는 약간의 딜레이의 원인을 모르겠습니다.
  • 이 부분도 1번 궁금증과 같은 부분에서 리뷰 달았습니다ㅎㅎ
    같이 확인 부탁드립니다..!
    그리고 추측하시는 이유가 맞을것 같습니다👍
  1. 네트워크를 통해 리스트를 불러올 때 현재의 위치가 적절한건지 궁금합니다.
  • 네! 저는 현재론 적절하다고 봅니다ㅎㅎ
    그런데 만약에 상품목록이 정말 정말 많으면 모두 전달받길 기다렸다 셀을 구성하게 될까요?
    pagination이 필요할수도 있을것 같습니다.
    (물론 이 부분은 지금 리팩토링 하실 부분은 아닐것 같습니다만 어떤 개념인지 봐도 좋을 포인트 같습니다🙌)

추가로 보너스처럼 구현되면 좋을것 같은 부분

  • 사소한거긴 한데 현재 화면회전 시 뷰가 원하는데로 노출되지 않고 오토레이아웃이 무너집니다.
    물론 요구사항에는 화면 회전에 대한 언급이 없기에 구현을 안하시는게 맞긴합니다!
    다만 그렇다면 landscape 모드에 대해서는 지원하지 않도록 하는게 더 앱 사용하는데서는 좋을것 같아 코멘트를 남겨봅니다🙋🏻

다시 한번 스텝2 뷰 참... 어려운.. 뷰.. 항상 해도 어려운.. 뷰..를 하시느라 고생많으셨어요!
잘하셔서 큰 수정사항은 없을것 같습니다ㅎㅎ
한번 확인 해보시고 재요청 혹은 머지 요청을 해주세요🙌

{
"images" : [
{
"filename" : "image__b0m6m3r8sv6q_large_2x.jpg",
Copy link
Member

Choose a reason for hiding this comment

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

추후 에셋이 많아진다면 더 명확한 이미지 파일 네이밍이 필요할거 같습니다🙋🏻

Choose a reason for hiding this comment

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

네 mac mini, mac book으로 네이밍 변경했습니다~

Comment on lines 9 to 14
"idiom" : "universal",
"scale" : "2x"
},
{
"idiom" : "universal",
"scale" : "3x"
Copy link
Member

Choose a reason for hiding this comment

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

2x, 3x 스케일에는 파일이 안들어가도 괜찮은걸까요?🙋🏻

Copy link

@llghdud921 llghdud921 Jan 12, 2022

Choose a reason for hiding this comment

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

ios에서 assets 기능에 스케일에 대한 기능을 처음 알았습니다 :) 감사합니다.
jpeg, jpg 형식의 비트 기반의 이미지는 이미지 변환 사이트를 이용해서 각각의 스케일 파일을 넣어줄 수 있을것 같습니다.

Comment on lines 2 to 21
"images" : [
{
"filename" : "mbp14-spacegray-select-202110_GEO_KR.jpeg",
"idiom" : "universal",
"scale" : "1x"
},
{
"idiom" : "universal",
"scale" : "2x"
},
{
"idiom" : "universal",
"scale" : "3x"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Scales Inspector 속성에 Single Scales와 Individual Scales가 있는데 어떤 차이가 있으며 어떨때 어떻게 옵션을 주면 좋을까요?
이와 관련해서 들어올 에셋의 파일 형식(확장자)를 보고 생각해볼 수 있겠습니다.
흔히 아이콘, 버튼, 로고 등에서 SVG 파일을 현업에서는 많이 사용하고 있는데 왜 사용하는건지 다른 jpg, jpeg, pdf 등의 형식과 어떤 차이가 있는지 살펴보면 좋을것 같습니다🙋🏻
분명 클라 개발자로써 많은 도움이 될것이며 디자인 파트와 소통할때도 중요한 부분이 될겁니다ㅎㅎ
스크린샷 2022-01-11 오후 4 15 39

Copy link
Member Author

Choose a reason for hiding this comment

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

오.. 이런 디테일 적인 부분까지.. 감사합니다.
알아보니 크기별로 사진의 해상도를 어떤 기기이든 상관없이 사용자가 사용하는 기기 기반으로 최적화를 할 수 있는 기능인 것 같습니다.
따라서 Individual Scales과 Single Scales의 차이는...
Vector 기반은 Single Scales로 설정해야하고,
bit 기반은 Single Scale로 설정하는 것 같습니다.
SVG는 2차원 벡터 그래픽을 표현하기 위한 XML 기반의 파일 형식인데,
벡터이미지는 특성상 확대를 해도 픽셀이 깨지지 않기 때문에 현업에서 많이 사용하는 것 같네요.
저희가 현재 추가해둔 이미지는 단순히 셀의 샘플용 이미지이기 때문에 Single Scales로 설정해주고 Preserve Vactor Data를 체크해주었는데, 맞게 설정한 것일까요?

Copy link
Member

Choose a reason for hiding this comment

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

네 현재로썬 괜찮습니다!

@IBOutlet private var stockLabel: UILabel!

static let listIdentifier = "ListView"
static let gridItentifier = "GridView"
Copy link
Member

Choose a reason for hiding this comment

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

오타가 있습니다🙋🏻
gridItentifier > gridIdentifier를 의도하신것 같습니다~!

Choose a reason for hiding this comment

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

네 수정완료했습니다!!!!!!

priceLabels = [priceLabel, discountPriceLabel, stockLabel]
}

func productConfigure(product: Product) {
Copy link
Member

Choose a reason for hiding this comment

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

보통 메서드는 동사+명사 형태를 띄는걸로 저는 알고 있습니다!
configureProduct가 조금 더 어색하지 않다고 보이는데 두분의 의견은 어떠신가요?

Copy link
Member

Choose a reason for hiding this comment

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

추가로 뒤 파라미터 인자 또한 메서드 네이밍 시그니쳐로 볼 수 있습니다.
이 경우 해당 메서드는 productConfigureProduct로 해석이 됩니다.
조금 어색하죠..?
이럴때는 of와 같은 label을 사용함으로 자연스럽게 만드는걸 추천드립니다🙋🏻

Choose a reason for hiding this comment

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

configureProduct로 네이밍 변경해주었고 styleConfigure 또한 cofigureStyle로 변경했습니다.
매개변수에 label을 붙이는 것이 훨씬 가독성있고 자연스러운것같습니다ㅎㅎ 감사합니다

Comment on lines +10 to +19
extension String {
var strikeThrough: NSAttributedString {
let attributeString: NSMutableAttributedString = NSMutableAttributedString(string: self)
attributeString.addAttribute(
NSAttributedString.Key.strikethroughStyle,
value: NSUnderlineStyle.single.rawValue,
range: NSRange(location: 0, length: attributeString.length)
)
return attributeString
}
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 +11 to +14
func scrollToTop() {
let topOffset = CGPoint(x: 0, y: -contentInset.top)
setContentOffset(topOffset, animated: false)
}
Copy link
Member

Choose a reason for hiding this comment

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

오 뷰전환 시 상단으로 스크롤 되는 좋은 기능도 직접 구현하셨네요!👍👍

case .success(let data):
encodeData = data
case .failure:
guard let encodeData = jsonEncode(data: data) else {
Copy link
Member

Choose a reason for hiding this comment

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

인코딩된 데이터의 의미로 encodedData라는 네이밍은 어떨까요?🙋🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

아...!! 그렇네요. ㅎㅎ 이부분도 피드백 반영하여 수정하겠습니다!

}
let identifier = "cd706a3e-66db-11ec-9626-796401f2341a"
Copy link
Member

Choose a reason for hiding this comment

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

이 프로퍼티는 private하면 좋을것 같습니다🙋🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

오 위에 프로퍼티들도 다 접근제어가 안붙어있네요. 감사합니다 😁

@@ -25,6 +25,12 @@
+ [의문점](#1-2-의문점)
+ [Trouble Shooting](#1-3-Trouble-Shooting)
+ [배운 개념](#1-4-배운-개념)
+ [PR 후 개선사항](#1-5-PR-후-개선사항)
- [STEP 2 : 상품 목록 화면 구현](#STEP-2--상품-목록-화면-구현)
Copy link
Member

Choose a reason for hiding this comment

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

크 스텝2도 역시 소홀히 하지 않으셨군요!
최고입니다👍👍👍

leeari95 and others added 4 commits January 12, 2022 14:32
- 단순 샘플용 이미지기 때문에 Scales를 individual에서 single로 수정
- 파일이름을 식별 가능한 이름으로 수정
- configureProduct, configureStyle로 네이밍 변경 및 매개변수 label of 설정
- gridIdentifier으로 네이밍 변경
- formattedString으로 네이밍 변경
- encodedData로 네이밍 변경
- width로 네이밍 변경
leeari95 and others added 3 commits January 12, 2022 14:54
- ProductsCollectionView 내부에 레이아웃 관련 프로퍼티 추가
- 기존의 listFlowLayout, gridFlowLayout 삭제
- cellstyle에 따른 itemsize 조절하는 cellSize() 구현
- list, grid마다 갖고있는 속성값 프로퍼티로 정의
@leeari95
Copy link
Member Author

안녕하세요. 그린! 이번에도 꼼꼼하고 세심한 피드백 감사드립니다. 😁
피드백 반영하여 수정한 사항은 아래와 같습니다!

수정사항

  • Asset에 등록되어있는 이미지 설정값 수정
  • 전반적인 네이밍 수정
  • 삼항연산자로 조건문 개선
  • 빠져있는 접근제어 추가
  • 동적으로 레이아웃을 잡을 수 있도록 UICollectionViewDelegateFlowLayout을 채택
    • 가로모드, 세로모드에서도 레이아웃이 뭉개지지 않도록 개선

피드백 주신 부분 답변

requestListSearch(page: 1, itemsPerPage: 10)해당 메서드 호출 시 인자는 고정적으로 저렇게 주는건지 궁금합니다🙋🏻

서버 API 문서의 예시를 보고 인자를 주었습니다ㅎㅎ
근데 다른 page 값을 넣어도 동일한 파일들로 나오더라구요.. 그래서 기본값으로 넣어주었습니다 🙂

현재 최종 코드에서는 reloadData 메서드 호출을 분기에서 각 처리 후 마지막에 두신것 같아요.근데 위에서 언급해주셨을때는 각 분기안에 들어가있습니다.어떤 차이가 있으며 파생되는 결과가 어떤게 있을까요?🤔

각 분기에 들어가는 것은 코드가 중복되기 때문에 마지막에 두었었는데요. 어디에 두냐에 따라 어떤 차이가 있는지... 감이 잘 오지 않습니다 😭
그리고 reloadData를 최상단으로 올렸더니 문제가 해결되었어요...
근데 왜 되는지 이유를 못찾겠어요... 도와주세요...! 그린!!

얼럿이 나타나도록 테스트를 혹시 해보셨을까요?

네! 이 부분은 실패하는 결과로 세팅한 후 얼럿이 잘 뜨는지 테스트 해보았습니다.
fatalError()를 사용해주는 곳은 단순히 옵셔널 바인딩 해주는 부분이라서 얼럿 없이 처리를 해주었는데요.
이 부분에도 얼럿을 띄우는게 맞나? 싶네요.. 셀이 불러와질때마다 얼럿이 뜰 상황을 생각하니 끔찍한데요...

혹시 화면전환에서 Navigation 방식과 Modal 방식을 선택하는 가장 큰 기준점이 어떤게 있을까요?🙋🏻

이전 viewController의 데이터가 전달되는 경우 navigation을 선택하고 이전 viewController의 데이터를 전달하기 보다 추가, 수정하는 경우 modal을 선택하는 것 같습니다.
현재 상품등록은 데이터를 추가해주는 창으로 modal 방식으로 구현하였습니다.

Copy link
Member

@GREENOVER GREENOVER left a comment

Choose a reason for hiding this comment

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

@leeari95 @llghdud921
고생하셨습니다!
정말이지.. 최고네요👍
화면회전에 대해서도 아주 잘 추가적으로 구현해주셔서 더욱 완성도 있는 앱이 되었어요!
우선 리뷰 코멘트를 추가적으로 몇개를 달아봤습니다.
또한 기능 QA중 하나 궁금한게 있습니다.
혹시 디바이스별로 화면 회전에 대해서 테스트를 해보셨을까요?
아마 디바이스 사이즈가 작은 기기로 가로 리스트를 보여주게 되면 아래와 같이 좌우 여백이 많이 생기게 됩니다.
혹시 이는 의도하신건지 그게 아니라면 코드 수정이 필요해보여서 여쭤봅니다🙋🏻
Simulator Screen Shot - iPhone SE (2nd generation) - 2022-01-12 at 22 01 31
그 외에는 기능 동작 전부 이상없습니다👍


질문에 대한 저의 의견

  1. reloadData()
  • 디코 디엠으로도 의견을 아주 짧게 나눠봤지만 코드상에서 flawlayout을 구성하며 delegate의 추가 구현이 있었는데요.
    기존에는 디바이스에 오토레이아웃을 초기에는 잡을 수 있지만 화면 전환 시 레이아웃이 잡히는것이 아주 잠깐이지만
    포커싱될 수 없어서 경고 메시지가 떴을것 같습니다.
    이에 그때 몇 UI요소들의 레이아웃이 전환 시 결정되지 않았기에 에러 메시지가 나타났는데 이는 시간될때 같이 회의실에서
    해당 UI 요소의 메모리 주소값을 가지고 같이 뷰 디버깅해보면서 찾아보면 좋을것 같습니다.
  1. 얼럿 관련
  • 앗 말씀하신데로 그럴 경우는 최악이겠네요.
    그렇다면 여러 방법이 있을것 같은데 한번만 얼럿을 띄우도록도 해볼 수 있지 않을까요?
    그렇게하려면 메서드 로직을 추가해서 전달해줘야할것 같습니다.
    근데 사실 너무 추가적인 구현이 되는것 같아서 시간이 되시면 해보고 아니면 다른것부터 힘을 실어보는게 좋을것 같아요.
    (보너스 스텝도 한번 해보셔야하니..!?)

다시 한번 고생하셨어요!
저는 마이너한 부분빼고는 머지해도 될것 같은데 한번 마지막으로 보시고 마이너한 수정사항들이라 생각되면
보너스 스텝을 진행하면서 같이 하도록 머지 요청을 아니면 수정을 해주시고 재요청을 해주시면 되겠습니다👍
그렇기에 request Changes로는 넘기지 않고 코멘트로 달아두겠습니다🙌
(크리티컬한 변경 요청할 부분이 없습니다.)

{
"images" : [
{
"filename" : "mac book.jpeg",
Copy link
Member

Choose a reason for hiding this comment

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

저는 가급적 공백을 포함하지 않는것을 선호합니다🙋🏻
이유는 추후 해당 코드에서 직접 파일이 쓰이거나 혹은 R.generated 파일을 이용할때도 공백이라면 만약 실수로 두칸이 들어가거나 했다면 어느 부분에서 실수했는지 찾기가 어렵기때문입니다.

Copy link
Member Author

@leeari95 leeari95 Jan 13, 2022

Choose a reason for hiding this comment

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

아.. 그렇군요. 의견 감사합니다 ☺️
의견 적극 반영하여 공백없이 파일명을 수정해보았습니다!

Comment on lines 89 to 92
for: indexPath) as? ProductCell else {
showAlert(message: Message.unknownError)
return UICollectionViewCell()
}
Copy link
Member

Choose a reason for hiding this comment

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

이 부분에서 가독이 조금 떨어지는것 같아요..!

guard let cell = collectionView.dequeueReusableCell(
    withReuseIdentifier: currentCellIdentifier,
    for: indexPath
) as? ProductCell 
else {
    showAlert(message: Message.unknownError)
    return UICollectionViewCell()
}

이런 코드는 어떠세요?

Copy link
Member Author

Choose a reason for hiding this comment

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

오.. 컨벤션을 이렇게 맞춰야하는건지 잘 모르고 그랬던거 같아요!
이 코드 이후에도 줄바꿈 하는 부분을 똑같은 컨벤션으로 맞춰주었습니다.
피드백 감사합니다! 🙇🏻‍♀️🙏🏻

Comment on lines 101 to 104
func collectionView(_ collectionView: UICollectionView,
layout collectionViewLayout: UICollectionViewLayout,
sizeForItemAt indexPath: IndexPath
) -> CGSize {
Copy link
Member

Choose a reason for hiding this comment

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

현재 이런 메서드 형태들도 이런 스타일이 많이 사용되고 있으며 가독성에서도 더 좋다고 생각합니다🙋🏻

func collectionView(
    _ collectionView: UICollectionView,
    layout collectionViewLayout: UICollectionViewLayout,
    sizeForItemAt indexPath: IndexPath
) -> CGSize {
...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

위와 같이 모두 수정처리 해주었습니다! ☺️

Comment on lines +80 to +82
if product.stock == .zero {
stockLabel.textColor = .systemOrange
stockLabel.text = "품절"
Copy link
Member

Choose a reason for hiding this comment

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

product stock이 0이면 품절을 잘 구현해주셨는데
stock을 가지고 이러한 품절과 같이 가공해주는건 모델, 뷰, 뷰컨트롤러 중 어디서 다뤄주는게 적합할까요?🙋🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

stock 가져오는 건 컨트롤러인데,
그 데이터에 따라서 label의 옵션을 바꿔주는 부분이니 View에서 다뤄주는 것이 적합하지 않을까라는 생각이 듭니다!

Copy link
Member

Choose a reason for hiding this comment

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

네 지금은 큰 문젠 없습니다!
다만 뷰에는 비지니스 로직이 들어가지 않는것이 좋습니다.
뷰는 정말 단순히 UI요소들을 올리는것 그 이상 그이하의 역할도 하지 않는것이 확실한 분리가 될것 같습니다.
이 경우는 판별하는 로직들이 들어가니 문제는 없긴하지만 저의 경우는 MVC에서는 모델 혹은 뷰컨에서 작업해주긴합니다!
다만 이건 MVC에서 주체를 확실히 할 수 없는 명확한 한계이자 문제점이기도하고 정답이 있는 부분은 아닙니다.
추후 MVVM이나 뷰,모델,컨트롤러가 뚜렷히 구분된 아키텍쳐를 공부하실때 적용해보면 좋을것 같네요🙌

register(listNibName, forCellWithReuseIdentifier: ProductCell.gridIdentifier)
}

private func setUpLayput() -> UICollectionViewFlowLayout {
Copy link
Member

Choose a reason for hiding this comment

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

Layput > 'Layout' 오타인것 같습니다🙋🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

헉.. 오타를 찾아주셔서 감사합니다!!! 😂🙇🏻‍♀️

Comment on lines +89 to +90
return CGSize(width: (screenWidth / 2) * 0.93,
height: (screenWidth / 2) * 1.32)
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 Author

Choose a reason for hiding this comment

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

마음에 드는 사이즈가 나올 때 까지 Run을 돌려가면서 작업을 했던 것 같습니다 😂

@llghdud921
Copy link

와우.. 추가 피드백까지.. 항상 그린의 꼼꼼함에 감탄합니다!
추가 의견에 대해서 답변 한번 드리도록 해보겠습니다. 😊

디바이스 별로 화면 회전에 대한 테스트

네 그린의 말씀을 듣고 테스트를 진행해보았는데요. 노치가 존재하는 디바이스(아이폰 8, 아이폰 se 등)에서는 화면 회전시에 sizeForItemAt을 불러오지 않는데서 비롯된 문제인 것 같아요.

실제 디버깅 테스트를 해본 결과 portrait → landscape right로 회전 시에 sizeForItemAt 메서드가 호출되지 않았습니다.
다 회전하고 마지막으로 landScape → Protrait로 회전시 그제서야 호출이 되더라구요...?
이 부분까지 케어하려면 해줄 수는 있지만 오버엔지리어닝[?] 같다는 생각이 들기도 하고, 너무 코드가 길어질 것 같아서 현재 코드 유지하기로 했습니다. 😊

얼럿 관련

이 부분을 indexPath.item이 0인 경우에만 showAlert을 띄우도록 하려고 하는데 그린 생각은 어떠신지 의견이 궁금합니다!

func collectionView(
    _ collectionView: UICollectionView,
    cellForItemAt indexPath: IndexPath
) -> UICollectionViewCell {
    guard let cell = collectionView.dequeueReusableCell(
        withReuseIdentifier: currentCellIdentifier,
        for: indexPath
    ) as? ProductCell else {
        if indexPath.item == 0 { // 이부분이요!
            showAlert(message: Message.unknownError)
        }
        return UICollectionViewCell()
    }
    cell.configureStyle(of: currentCellIdentifier)
    cell.configureProduct(of: productList[indexPath.row])
    
    return cell
}

Copy link
Member

@GREENOVER GREENOVER left a comment

Choose a reason for hiding this comment

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

@leeari95 @llghdud921
고생하셨습니다!
더 가독성이 좋아졌네요ㅎㅎ
네! 디바이스 사이즈에 대한 모든 대응은 이번 프로젝트에 있는건 아니고 너무 잘 구현해주셔서 여쭤봤습니다!
지금은 다른거에 먼저 힘쓰는게 낫겠네요ㅎㅎ
추후 시간나면 리팩토링 해보셔도 좋을 부분인것 같아요!
또한 얼럿 관련해서 보여주신 코드 괜찮다고 생각합니다🙌
우선 지금으로써 저정도 처리만 해줘도 아주 양호한것 같네요!
벌써 시간이 목요일이네요🥲
보너스 스텝을 진행해보실 수 있도록 머지드릴께요!
(보너스 스텝 아니여도 충분히 머지 될 정도입니다👍)

@GREENOVER GREENOVER merged commit 38d1cf0 into yagom-academy:4-leeari95 Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants