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

[STEP2] 재근 에라 #4

Open
wants to merge 18 commits into
base: 1_jaegeun
Choose a base branch
from
Open

Conversation

wormsJJG
Copy link

@wormsJJG wormsJJG commented Nov 4, 2022

완성화면

2022-11-04.3.57.18.mov

ActivityIndicator 테스트

2022-11-04.4.01.06.mov

캐시 구현

  • 캐시는 이미지를 저장하는 목적으로 사용하였습니다. 텍스트를 저장하지 않은 이유는 데이터 크기가 그렇게 많지 않기 때문입니다.
  • 이미지를 캐시에 담지 않는다면 셀 이미지 지연로딩 현상이 발생하게 됩니다.

스크린샷 2022-11-04 오후 4 11 12

  • 데이터를 가져오기전 캐시에 url키 값으로 된 이미지가 있으면 캐시에 있는 이미지를 반환, 없으면 url을 통해 이미지를 가지고 오게 됩니다.

Modern CollectionView (List, Grid) 구현 (끝판왕 진짜 어려웠어요)

  • 저희는 List, Grid를 두개의 컬렉션뷰로 나누니 않으려고 고민했고 결론적으로 컬렉션 뷰의 레이아웃만 바꿔주어 해결하였습니다. 그러기 위해서는
    listLayout과 gridLayout을 따로 만들어 관리를 하고 Layout을 바꿔주는 기능을 해주는 함수가 필요했습니다. 그래서 CustomCollectionView를 만들었습니다.

새로운 구현 방식

Modern CollectionView는 원래 만들던 CollectionView와는 다르게 dataSource라는 프러퍼티를 따로 만들어 셀들을 관리 하는 것 이였습니다.
그래서 DataSource의 타입은 UICollectionViewDiffableDataSource<Section, Page> 로 지정하였습니다.
그리고 segmentControl의 선택된 인덱스를 통해 레이아웃을 관리 하였습니다.

스크린샷 2022-11-04 오후 4 17 39

이 컬렉션 뷰에서 가장 중요한건 snapShot을 넣어주는 부분이라고 생각했습니다.
section과 데이터를 설정해주고, 설정이 끝나면 액션인디케이터를 바꿔주는 함수입니다.

아쉬운 점

  • POP의 미숙함
    POP지향 프로그래밍을 해야지 해야지 하는데 실상 그렇게 하지 못했다는게 너무 아쉬웠습니다. 앞으로는 타입설계에 굉장히 많은 시간을 가지고 프로젝트를 실행해야겠다는 것을 느꼈습니다.
  • Cell 액세서리 위치 조정

스크린샷 2022-11-04 오후 4 20 27

현재 셀 액세서리는 구름의 예시화면의 위치로 조정하지 못했습니다. 이것을 조정하려면 stack에 이미지와 뷰를 넣어 위치를 맞쳐 커스텀 액세서리로 넣어야 했고 이 작업이 까다로워 시도하지 못했습니다.

Step1에서 바뀐 통신 관련 타입

  • Step1 에서는 열거형 Request에서 URL반환까지의 일을 모두 맡아서 진행하였습니다. 하지만 그렇게 진행하였을 때 유지보수가 힘든 점이 있다는 걸 파악후 변경하였습니다.
  • 세분화

스크린샷 2022-11-04 오후 4 22 42

Request 타입은 이제 그저 무슨 결과를 받을건지에 관한 타입만 관리 하게 됩니다.

  • URLComponents 사용

스크린샷 2022-11-04 오후 4 23 34

URLComponents를 사용하여 URL을 좀더 세분화하고 유지보수 하기 쉽게 구현하였습니다.

Copy link
Contributor

@jaemuYeo jaemuYeo left a comment

Choose a reason for hiding this comment

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

재근 에라 스텝2도 멋지게 해주었군요!!!
코드를 통해 몇가지 내용들 전달해드렸고 나머지 내용은 이 곳에 작성해놓겠습니다~

셀 선택 후 셀선택 제거

현재 특정 셀을 선택후 화면 이동하거나 그 페이지를 볼때 계속 선택된 형태를 보입니다
어떻게 없애볼수 있을까요??

ezgif com-gif-maker (6)

인디케이터 활용

현재 인디케이터는 앱이 실행하고 잠깐의 데이터 로드를 표시하고있어요~
그 외에 gif 처럼 사용자가 새로고침을 하려는 행동을 했을때
또는 list와 grid를 변경할때 인디케이터를 추가해보면 좋을 것 같아요!
ezgif com-gif-maker (7)

셀 재사용 문제

ezgif com-gif-maker (5)
현재 아래로 스크롤했다가 올리면 이미지들이 바뀌는 것을 확인했습니다!
어떤 곳이 문제일까요 🤔


import UIKit

final class MainViewController: UIViewController, UIGestureRecognizerDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

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

final 👍
해당 VC에서 UIGestureRecognizerDelegate를 채택해놓고 사용안하였나요??
만약 구현한다면 저는 Delegate는 extension을 통해 구현하는 편입니다 🙂

MainVC는 너무 범용적이고 언제 main뷰가 바뀔지 모른다는 생각이 듭니다.
해당 뷰에 맞게 좀더 확실한 네이밍이었으면 좋겠어요!!

혹시 MARK 주석에 대해 알고계신가요??
조사해보고 해당 파일에 적용해봐도 좋을 것같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

현재 UIGestureRecognizerDelegate는 채택 후 관련 메서드를 사용하지 않아 extension으로 따로 빼두지 않았습니다. 하지만 UIGestureRecognizerDelegate를 채택하지 않는다면 스와이프 액션을 인지하지 못하는 문제가 생기게 됩니다

}

@IBOutlet weak private var viewTypeSegmentControl: UISegmentedControl!
@IBOutlet weak private var plusButton: UIBarButtonItem!
Copy link
Contributor

Choose a reason for hiding this comment

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

plusButton이라는 네이밍보다는 특정 화면으로 이동하는 네이밍으로 해주는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

moveToAddViewButton
어떠신가요 ㅎㅎ

@IBOutlet weak private var plusButton: UIBarButtonItem!
@IBOutlet weak private var pageCollectionView: CustomCollectionView!

private let listCellID: String = "ListPageCell"
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 셀의 identifier는 커스텀셀에서 타입 프로퍼티로 관리하거나 따로 셀의 아이디를 가지는 타입을 만들어 보관하는 방향을
지향합니다! VC에서 셀의 아이디를 갖고있는것에 대해 어떻게 생각하나요?

Copy link
Author

Choose a reason for hiding this comment

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

ccellID 따로 관리하는게 좋을 것 같다고 생각되네요


private let listCellID: String = "ListPageCell"
private let gridCellID: String = "GridPageCell"
private let networkManager = NetworkManager()
Copy link
Contributor

Choose a reason for hiding this comment

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

API통신을 위한 네트워크 구현부는 인스턴스화 하지 않는답니다:)
그 이유는 무엇일까요??

Copy link
Author

Choose a reason for hiding this comment

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

네트워크 관련 로직은 굉장히 복잡해서 인스턴스화하여 관리하지 않는 것으로 알고 있습니다. 하지만 지금 앱 사양에서 static으로 하지않아도 될 것 같다는 생각이 들었습니다. 잼킹은 어떻게 생각하시나요?

private let swipeGesture = UISwipeGestureRecognizer()
private var productListPage: ProductListPage?

private lazy var dataSource = UICollectionViewDiffableDataSource<Section, Page>(collectionView: pageCollectionView) { pageCollectionView, indexPath, itemIdentifier in
Copy link
Contributor

Choose a reason for hiding this comment

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

dataSource는 어떤 dataSource인가요?
저는 이 부분은 메서드로 구현해도 좋을 것 같다 생각합니다!

어떨때 연산프로퍼티를 사용하고 메서드를 사용하는지 두 종류의 차이는 무엇인지 조사해보면 좋을 것같아요!!

@@ -31,7 +33,7 @@ final class NetworkManager {
}

func getData(requestType: Request, completion: @escaping(Result<Data, NetworkError>) -> Void ) {
guard let url = URL(string: requestType.url) else {
guard let url = urlManager.requestURL(requestType: requestType)?.url else {
return completion(.failure(NetworkError.invalidURL))
}
var request = URLRequest(url: url)
Copy link
Contributor

Choose a reason for hiding this comment

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

요 아래 request.httpMethod = HTTPMethod.get 부분은 있어도 없어도 실행해 문제는 없는데
get으로 해준 이유가 있나요???

현재는 HTTPMethod 타입이 불필요해 보여서요 🤔

Copy link

Choose a reason for hiding this comment

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

추후 추가되는 새로운 기능을 위해 만들어뒀습니다!

Copy link
Author

Choose a reason for hiding this comment

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

추후 추가되는 기능을 대비해 만들었습니다.


import UIKit

struct URLManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

크~~ URLComponents 성공했군요 👍

Copy link
Author

Choose a reason for hiding this comment

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

😻


import UIKit

protocol ImageCacheable {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Author

Choose a reason for hiding this comment

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

🫢

case grid = 1
}

private lazy var listLayout: UICollectionViewCompositionalLayout = {
Copy link
Contributor

Choose a reason for hiding this comment

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

list는 compositionalLayout으로 grid는 flowLayout으로 설정한 이유가 무엇인가요??

Copy link
Author

Choose a reason for hiding this comment

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

컴포지셔널 레이아웃에 리스트형식을 제공하기 때문입니다
그리드 형식은 커스텀으로 만드는 셀 이기 때문에 그리드는 플로우 레이아웃을 채택하였습니다

return layout
}()

func changeLayout(type: ViewType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Layout이 변화할때 자연스럽게 애니메이션을 주어 변경되는 방법은 없을까요??
ezgif com-gif-maker

현재는 변화하는 과정에서 뚝뚝 끊겨보여요 🤔

Copy link
Author

Choose a reason for hiding this comment

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

음... 생각해보겠습니다

jaemuYeo pushed a commit that referenced this pull request Nov 10, 2022
@wormsJJG wormsJJG requested a review from jaemuYeo November 10, 2022 03:06
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.

3 participants