오픈마켓2 [STEP 1] 아리, 제리 #109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- ImageCell 타입과 XIB파일 생성 - 셀의 레이아웃을 설정하기 위해 FlowLayout 설정 - 컬렉션뷰 스크롤바를 제거
- ImageAddHeaderView 타입과 Xib 생성 - 셀의 레이아웃 설정하기 위해 FlowLayout 설정
- NotificationCenter를 활용하여 headerView의 이벤트를 ViewController에 전달 - 이미지를 추가한 후 performBatchUpdates 메소드를 활용하여 collectionView 셀 추가
- Notification.name extension 추가 - ImageAddHeaderView에 updateLabelText() 추가 - ImageAddHeaderView의 button에서 button과Label로 구성 변경
- UIImage extension하여 resized, compress 메소드 생성
- Done버튼을 눌렀을 때 이미지가 없을 경우 얼럿을 띄우도록 구현 - 기존 Alert 메세지 Message 타입부분으로 리팩토링 - add버튼 클릭시 full screen으로 뜨도록 구현
- 가독성을 높이기 위해 조건문을 따로 변수로 할당 - 메세지 부분 줄바꿈 추가 수정
- 글씨가 실시간으로 커질 수 있도록 다이나믹 타입 적용
- identifier 변경 - ProductRegistration 생성하는 기능 추가
- UIViewController+extension 수정 - request와 네트워킹 하는 부분을 메소드로 분리
- RegisterProduct에 ScrollView를 배경으로 깔도록 구현 - 스택뷰의 높이의 우선순위를 낮추고 텍스트뷰의 높이 우선순위를 높임 - 텍스트뷰의 스크롤 기능을 제거 - 텍스트뷰의 높이가 늘어날 수록 스택뷰의 높이도 늘어나도록 오토레이아웃 수정 - NotificationCenter를 활용하여 키보드가 나타날 때 콘텐츠를 가리지 않도록 메서드 추가 - 스크롤을 하거나 텍스트필드 사이를 터치했을 때 키보드가 내려가도록 기능 추가
- ImageAddHeaderViewDelegate 생성 - 기존의 notification에서 delegate pattern으로 수정
- 기존 seyUpData 메소드를 삭제하고 requestProducts 메소드에 completion 파라미터 추가 - requestProducts를 호출하는 곳에서 다른 로직으로 구성하도록 수정 - MainViewController에 NotificationCenter를 추가 - 기존 RegisterViewController에 dismiss부분 리팩토링 - 사용하지 않는 Notification.Name 삭제
- DetailViewController 생성 - IBOutlet, IBAction 연결 - MainViewController에 CollectionView delegate 메소드 추가 - performSegue를 활용하여 뷰를 전환하도록 구성
- product 받아서 label text 띄우는 기능
- Segue가 정상적이지 않은 부분 if문으로 분기처리하여 개선
- RegisterDataSource class 생성하여 기존의 CollectionViewDataSource 내 코드 분리 - RegisterDataSource에 images 등 데이터 다루는 코드 분리 - delegate pattern 적용된 부분 notification 적용으로 개선
- DetailViewController에서 뷰가 전환될 때 데이터를 전달하도록 prepare 메소드 추가 - RegisterViewController에 모드 타입을 추가 - 모드에 따라 뷰의 동작 일부분을 작동하지 않도록 guard문 추가 - 모드가 modify이면 전달받은 상품정보로 뷰를 구성하도록 추가
- notification deinit 코드 제거
- product 데이터를 전달받아서 이미지를 캐싱한 후 images에 append하도록 구현
- 얼럿을 통해 비밀번호 여부를 확인하는 코드 추가 - 컨벤션 안맞는 부분 수정처리 - UIViewController+extension에 alertInputPassword 메소드 추가 - ProductModification에 extension으로 이니셜라이저 추가
- 폴더 및 ViewController, Storyboard 네이밍을 수정 - EditViewController 내부 접근제어 추가 - 메소드를 호출 순서로 위치를 조정하여 개선
- MainViewController 호출순서로 코드 순서위치 변경
- 접근제어 추가 - 메소드 순서를 호출되는 순서, 연관성으로 개선
- Alertmesssage enum namespace 생성 및 관련 코드 수정 - AlertConstatnt enum namespace 생성 및 관련 코드 수정 - EditMode enum namespace 생성 및 관련 코드 수정 - SegueIdentifier enum namespace 생성 및 관련 코드 수정 - 상품 수정, 등록 title 관련 로직 수정
- alert Action Sheet를 UIViewController+extension 파일에 메소드로 분리 - alertActionSheet 메소드를 활용하여 상품수정 부분 개선 - DetailViewController에 secret 프로퍼티를 추가하고, prepare시 넘겨주도록 개선
GREENOVER
requested changes
Jan 21, 2022
There was a problem hiding this comment.
@leeari95 @llghdud921
역시 이번에도 빠르시군요!
너무 고생 많으셨습니다🤭
정확한 기한을 잘 지켜주셨네요👍
전체적으로 로직도 잘 나눠졌고 코드 논리상으로도 문제가 없기에 이번 PR은 주로 사용자 입장에서
기능에 대해 더 보완되면 좋을점들을 리뷰해봤습니다🙌
칭찬 드리고 싶은 부분
- 전체적인 View와 VC 이외에도 로직과 파일들을 잘 쪼개고 나눠주셨네요 💯
- 기능적인 부분에서도 마이너한 몇가지 제외하곤 전부 편리하게 작동하네요 💯
- 리드미 작성 역시나 최고 💯
고민하셨던 부분에 대한 저의 의견
- 고민하셨던 부분에 대한 목차들을 쭉 보고 코드로도 봤는데 전부 이견 없습니다.
너무 잘 구현해주셨고 더 첨언 드릴 말씀은 없어서 이렇게 의견아닌 의견을 남겨봅니다😅
다소 마이너한 부분에서의 궁금증이나 의견은 리뷰 코멘트를 봐주시면 될것 같습니다!
조언이 필요하신 부분에 대한 저의 의견
- Delegate vs Notification
- 저는 이 코드에서 노티피케이션을 사용하면서 어색함은 찾지 못했어요!
딜리게이트로 구현을 하기엔 조금 다소 껄끄럽거나 역할이 분명하지 못한 애들도 있다고 보여서 노티피케이션을 활용하신점 좋은것 같습니다.
현재 MVC구조에서는 이 구현이 가장 깔끔했지 않을까 싶습니다.
- ViewController에 addGestureRecognizer를 하면 발생하는 증상
- 아마도 요런 부분을 키워드로 한번 찾아보시고 적용해보시면 어떨까합니다.
becomeFirstResponder와resignFirstResponder
아마 추측으로는 resonder chain이 뭔가 꼬였다고 생각들어요.
그래서 저는 구현할때 지금 구조와는 다르지만 저 키보드를 특정 뷰컨에 국한짓지 않고 아래와 같이
public func hideKeyboard() {
UIApplication.shared.sendAction(
#selector(UIResponder.resignFirstResponder), to: nil, from: nil, for: nil
)
}요런식으로 전역적으로 가져가 이게 그 특정한 뷰에서 나타나는게 아닌 총 AppView에서 공통적인 기능처럼 쓰이도록 해줍니다.
3. Index out of range
- 음 고민으로 말씀하신것처럼 저도 드는 생각이 네트워킹보다 뷰를 그려주는 로직이 빠르게 동작해서 그런거외엔 없을것 같다고 보여요.
동기적으로 이를 처리해보면 어떨까 생각이 듭니다.
흠.. 조금 더 고민해봐야겠습니다. 지금 코드에서는 딱 어떻게 해야할지 감이 잘 안잡히네요🥲
- DataSource를 ViewController와 분리
- 좋은데요?!ㅎㅎ
지금 꽤나 뷰컨을 다이어트 시키려고 파일로 많이 잘 구조적으로 분리해주셨단 생각이 들어요.
이에 저도 찬성합니다.
- 이미지 캐싱
- 네 현재 코드로는 딱히 구현이 잘못되었거나 한 부분은 없다고 보입니다..!
궁금한건 디테일로 들어가기전 상품 리스트에서 이미 이미지들을 캐싱하는것 같은데 디테일로 들어갔을때
이미지 캐싱이 놓쳤던 부분이 있다고 판단해서 그럴 시에 다시 캐싱을 해주도록 한건가요?
요 부분은 스텝2 디테일 화면 진행하시면서 더 봐보고 얘기해보면 될것 같아요🙌
- UIImagePickerController
- 사용하신 UIImagePicker는 아시겠지만 단일 선택 밖에 안됩니다.
이는 어쩔수없는걸로 알고 있어요.
원하시는 멀티 선택을 하려면 PHPicker를 사용해야해요.
이건 iOS14 이상에서 쓸 수 있을거고 멀티 선택이 됩니다.
다만 크롭 기능이 없습니다ㅠ
저는 크롭 기능이 필요없어서 이를 쓰긴하는데요.. (저희 최소 지원도 14이상이라..)
사실 더 좋은 라이브러리들이 있기에 외부 라이브러리를 사용할 수 있다면 사용하는것도 맞는 방향인것 같습니다!
- View에 들어가는 문자열 관리
- 저는 네임스페이스 관리 좋아해요.
나중에 수정하기도 너무 용이하고..
다만 말씀하신데로 얼럿에 국한되었을때는 굳이 안써도될것 같아요.
얼럿에 액션 타이틀을 줄때는 네임스페이스로 불러오면 오히려 직관적이지 않을때도 있기에
저는 일단 얼럿 액션 타이틀을 줄때 네임스페이스로 관리해본적은 없습니다🤔
얼럿 자체를 하나의 타입으로 생성하고 이니셜라이저를 통해 인자를 넘겨주도록 그렇게 구현하는데,
기본적으로 들어갈 타이틀 값은 디폴트로 넣어주고 변경될 사항이 있다면 이니셜 시 전달해 변경만 해주고 있습니다!
전체적으로 로직도 잘 나눠져 리뷰하기에 편했습니다!
이번에는 여기보다 리뷰 코멘트로 세부적인 의견들을 많이 남겨봤습니다ㅎㅎ
이번에도 수고하셨어요!!
제가 생각할때는 기능상 어색한 부분이 있기에 보완되면 좋을것 같아서 우선 request changes로 남겨보겠씁니다🙌
수정되면 재요청 부탁드립니다🙏🏻
| extension Notification.Name { | ||
| static let addButton = Notification.Name("addButton") | ||
| static let editImageCountLabel = Notification.Name("editImageCountLabel") | ||
| static let updataMain = Notification.Name("updateMain") |
| import UIKit | ||
|
|
||
| extension UIImage { | ||
| func resized(percentage: CGFloat) -> UIImage { |
There was a problem hiding this comment.
혹시 이 메서드가 compress 외 다른 부분에서도 불릴 일이 있나요?🙋🏻
제한적이게 compress에서만 쓴다면 private하게 가도 될것 같아서요🤔
| import UIKit | ||
|
|
||
| extension UIImage { | ||
| func resized(percentage: CGFloat) -> UIImage { |
There was a problem hiding this comment.
resized는 리사이즈 되었다로 과거형으로 해석이 되는데 로직을 보면 리사이즈를 해주는 구현 같습니다.
이에 resize가 더 적합하지 않을까 싶습니다🙋🏻
| } | ||
| while compressedImageData.count > maxDataSize { | ||
| quality *= 0.8 | ||
| compressImage = compressImage.resized(percentage: quality) |
There was a problem hiding this comment.
compress된 이미지를 뜻하는 compressedImage가 더 적합한 네이밍이 아닐까 싶어요🤔
Comment on lines
11
to
31
| func resized(percentage: CGFloat) -> UIImage { | ||
| let size = CGSize(width: size.width * percentage, height: size.height * percentage) | ||
| return UIGraphicsImageRenderer(size: size, format: imageRendererFormat).image { _ in | ||
| draw(in: CGRect(origin: .zero, size: size)) | ||
| } | ||
| } | ||
|
|
||
| func compress() -> UIImage { | ||
| var compressImage = self | ||
| var quality: CGFloat = 1 | ||
| let maxDataSize = 307200 | ||
| guard var compressedImageData = compressImage.jpegData(compressionQuality: 1) else { | ||
| return UIImage() | ||
| } | ||
| while compressedImageData.count > maxDataSize { | ||
| quality *= 0.8 | ||
| compressImage = compressImage.resized(percentage: quality) | ||
| compressedImageData = compressImage.jpegData(compressionQuality: quality) ?? Data() | ||
| } | ||
| return compressImage | ||
| } |
There was a problem hiding this comment.
이미지 리사이징하는 일련의 로직 과정들이 유기롭고 좋네요👍
Comment on lines
34
to
36
| func collectionView( | ||
| _ collectionView: UICollectionView, | ||
| numberOfItemsInSection section: Int) -> Int { |
There was a problem hiding this comment.
여기도 아래와 같은 파라미터 코드 컨벤션으로 통일되면 더 보기 좋을것 같습니다🙋🏻
|
|
||
| func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) { | ||
| performSegue(withIdentifier: SegueIdentifier.productDetailView, sender: dataSource.productList[indexPath.item]) | ||
| collectionView.deselectItem(at: indexPath, animated: true) |
There was a problem hiding this comment.
deselectItem 이 메서드 왜 필요할까요?
없으면 원하는 동작이 안되었는지 궁금해요!
또 현재 셀을 선택할때 이게 내가 지금 터치한 즉 선택한 셀인지 음영표시 혹은 셀 전체의 색이 표시되야할것 같은데
안되더라구요ㅠ
구현사항은 아니지만 사용자 입장에서 필요하지 않을까 싶어요!
컬렉션뷰에서 이거 해주려면 어떻게 하면 좋을까요~?🤔
There was a problem hiding this comment.
오.. 찾아보니 Cell에 selectedBackgroundView가 있어서 그걸 활용해서 해결해보았습니다!
deselectItem는 TableView 시절 생각하고ㅋㅋㅋ 습관적으로 넣어버렸던 거 같네요.
의견 감사합니다~🙇🏻♀️
| static let wrongPassword = "비밀번호가 맞지 않습니다. 다시 시도해주세요." | ||
| static let editProduct = "상품을 편집하시겠습니까?" | ||
| static let dataDeliveredFail = "데이터 전달에 실패하였습니다." | ||
| static let productError = "상품을 불러오는데 문제가 발생했습니다." |
There was a problem hiding this comment.
fetchProductError같이 더 명확한 네이밍이면 추후 얼럿 케이스가 많아졌을때 헷갈리지 않을것 같습니다🙋🏻
| static let minimumImageCount = "이미지는 최소 1장 등록해야합니다." | ||
| static let maximumImageCount = "이미지는 최대 5개까지 첨부할 수 있어요" | ||
| static let minimumNameCount = "상품명이 세글자 이상이어야 합니다." | ||
| static let minimumPriceCount = "상품가격이 필수적으로 입력되어야 합니다." |
There was a problem hiding this comment.
필수적으로 입력되는건 minimum하고 조금 다른 맥락이라고 생각이 듭니다🤔
다른 네이밍은 없을까요?🥲
There was a problem hiding this comment.
@leeari95
Count라는것과 minimum은 이어진다고 보기에 count가 들어가는게 부적합하지 않나 생각이 듭니다🤔
저 에러 메시지 자체가 나타나는 원인이 어떤걸 세는게 아닌 단순 상품가격이 기입되지 않았기에 발생하는 메시지라고 보기에
requiredPrice로 충분할것 같아요!
| @@ -37,6 +37,12 @@ | |||
| + [Trouble Shooting](#3-3-Trouble-Shooting) | |||
| + [배운 개념](#3-4-배운-개념) | |||
| + [PR 후 개선사항](#3-5-PR-후-개선사항) | |||
| - [STEP 3 : 상품 등록/수정 화면 구현](#STEP-3--상품-등록/수정-화면-구현) | |||
There was a problem hiding this comment.
워후... 스텝3까지 아주아주 고생많으셨어요 그 와중에 리드미까지 짱짱입니다 두분👍👍👍
- alertInputPassword 내부에 텍스트 필드 글자를 암호화하도록 설정 - alertActionSheet에 누락된 deleteHandler를 파라미터 추가하여 개선
- HIG를 참고하여 얼럿시트의 버튼 순서를 변경 - 앨범에 접근할 때 보여지는 description을 재수정
- Notification을 활용하여 UIButton을 터치했을 때 이미지가 삭제될 수 있도록 개선
|
그린 ~ 이번에도 디테일하고 꼼꼼한.. 피드백 최고입니다! 수정사항
DM 드린 내용은 차주 평일 저녁에 한번 같이 봐주셨으면 합니다! 이 외 특이사항 없다면 merge 부탁드리겠습니다! |
GREENOVER
approved these changes
Jan 21, 2022
| @@ -3,7 +3,7 @@ | |||
| <plist version="1.0"> | |||
| <dict> | |||
| <key>NSPhotoLibraryAddUsageDescription</key> | |||
| <string>사진 앨범에 접근을 허용하시겠습니까?</string> | |||
| <string>사진 앨범에 접근하도록 허용버튼을 눌러주세요.</string> | |||
There was a problem hiding this comment.
사실 여담인데 현업에서는 왜 접근해야하는지 대략적인 이유를 나타내줍니다.
예를들어 "상품 이미지 등록을 위해 앨범 접근을 요청합니다" 요런식으로 주고 있긴합니다!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요. 그린 @GREENOVER
아리 @leeari95, 제리 @llghdud921 입니다!
오픈마켓2 Step1 구현을 마쳐 PR을 보냅니다.......
코드도 길어지고 기능이 많아져서 그런지 부족한 점이 많습니다 😅
상품 상세화면은 상품 수정을 위해 최소한으로만 구현을 해둔 상태이니 이 부분 참고 부탁드립니다...!!!!
혹시 부족한 점이나 놓친 부분이 있다면 번거롭더라도 한번 더 짚어주시면 감사하겠습니다. 🙇🏻♀️
이번 스텝도 잘부탁드릴게요~~~ 😃
기능 작동 예시
고민했던 점
1. 상품등록 화면 구현시 이미지를 추가하는 기능
UIGraphicsImageRenderer를 이용하여 20퍼센트씩 resize2. 입력 검증
3. RegisterViewController을 재활용
EditViewController네이밍을 지정하였습니다.4. DataSource를 ViewController와 분리
5. 키보드가 콘텐츠를 가리는 현상 해결
contentInset을keyboardFrame 사이즈의 높이만큼 버퍼를 추가하여 스크롤뷰를 확장하므로써 키보드 가림현상을 해결scrollIndicatorInsets도 같이 변경해주도록 하였습니다.ScrollEnabled기능을false로 비활성화priority를 조절.6. View와 Controller간의 소통
NotificationCenter를 사용하였습니다.7. 데이터가 변화할 때 Update하는 기능
request하여 데이터를 업데이트궁금했던 것 / 조언이 필요한 점
1. Delegate vs Notification
view와Controller사이에 소통하는 방식으로delegate pattern을 사용하였는데,DataSource파일을 분리하면서DataSource에서delegate객체를 이용하는 것에 한계를 느껴Notificationcenter를 사용하였습니다.NotificationCenter의 장점도 있지만 View와 Controller 간의 소통은 delegate를 많이 활용하는 것으로 알고있는데요. 현재 코드가 적절한지 아니면 또 다른 방법을 사용할 수 있을지 궁금합니다.2. ViewController에 addGestureRecognizer를 하면 발생하는 증상
키보드를 내리는 방법 중...
Recognizer방법을 사용하면 CollectionView의Delegate메소드 호출이 먹통이 되는데 어떤 문제인지 전혀 파악이 안되서 어려움을 겪었습니다.. 결국엔 해결방안을 찾지 못하고 다른 방법으로 문제를 해결하였는데요. 이 부분에 대해서 조언이 필요합니다.3. Index out of range
out of range에러가 나는데, 정확한 에러 원인을 찾지 못했습니다.4. 점점 비대해지는 ViewController를 어떻게 다이어트 시켜야하는지...
5. 이미지 캐싱
6. UIImagePickerController
7. View에 들어가는 문자열 관리
하하.. 적다보니 많아졌는데.. 이번에도 잘부탁드립니다~~~😄 🍏🙏🏻