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

Feat: 폴더 구독 기능 구현 #206

Open
wants to merge 15 commits into
base: refactor
Choose a base branch
from

Conversation

Hansangjin98
Copy link
Member

@Hansangjin98 Hansangjin98 commented Feb 13, 2024

📌 #205

📘 작업 유형

  • 신규 기능 추가
  • 버그 수정

📙 작업 내역

구현 내용 및 작업 내역을 기재합니다.

  • 폴더 구독 기능 구현
    • 신규 API 연결
    • 폴더 탭 -> 폴더 상세 뷰 진입시 권한에 따른 UI 및 기능 분리
  • 북마크 생성/편집 뷰 터치 버그 수정

📋 체크리스트

  • PR 제목에 Prefix(Feat, Refactor, Fix...etc) 와 작업 내용을 요약하여 기재했는가?
  • 코딩컨벤션을 준수하는가?
  • PR과 관련없는 변경사항이 없는가?
  • 내 코드에 대한 자기 검토가 되었는가?



Copy link
Contributor

@Jeongsuu Jeongsuu left a comment

Choose a reason for hiding this comment

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

굳굳 고생했어!👍


return FetchBookmarkResponse(
bookmarks: response.toDomain(),
currentPage: response.bookmarkListDTO.currentPage,
isLastPage: response.bookmarkListDTO.isLastPage
)
}

func subscribeFolder(id: Int) async throws {
try await networkProvider.request(endpoint: FolderEndpoint.subscribeFolder(id: id), type: APIResponse.self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. 구독 시작, 취소 API 호출 결과에 따라 "구독하기" 버튼, "구독 취소" 버튼 hidden 여부가 달라지는것 같은데 성공 여부를 체크하지 않아도 괜찮을까~?
이전에 있던 isSuccess 체크를 네트워크 레벨에서 체크하고 에러를 throwing 하는 방어 로직을 살리거나 하면 좋을듯!

Copy link
Member Author

Choose a reason for hiding this comment

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

체크 해야하는데 놓쳤네 ㅠㅠ
에러 검출이랑 핸들링 관련 로직 추가완료!

case .fetchBookmarkListInFolder(let id):
return AppProperties.baseURL + baseRouthPath + "/\(id)/bookmarks"
case .fetchBookmarkListInFolder(let id, let subscribe):
var finalPath = AppProperties.baseURL + baseRouthPath + "/\(id)/bookmarks"
Copy link
Contributor

Choose a reason for hiding this comment

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

baseRouthPath에서 Routh가 Route 인 것 같은데... 잘 모르겠닼ㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

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

오타난 코드를 계속 재활용하고 있었네 ㅋㅋㅋ 수정했음!

case .fetchBookmarkListInFolder(let id):
return AppProperties.baseURL + baseRouthPath + "/\(id)/bookmarks"
case .fetchBookmarkListInFolder(let id, let subscribe):
var finalPath = AppProperties.baseURL + baseRouthPath + "/\(id)/bookmarks"
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. 이 엔드포인트에서 path를 정의할때 매번 AppProperties.baseURL + baseRouthPath가 요구되는거면

위에서 정의한 baseRouthPath를 아래처럼 바꾸면 더 깔끔해질듯!

before

 var baseRouthPath: String {
    return "/app/folders"
  }

after

let basePath = AppProperties.baseURL + "/app/folders"

@@ -15,13 +15,18 @@ public struct Folder: Equatable {
public var title: String
public var color: String
public let count: Int
public var shared: Bool = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public var shared: Bool = false
public var isShared: Bool = false

Comment on lines 124 to 127
guard let viewMode = viewMode else {
return
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
guard let viewMode = viewMode else {
return
}
guard let viewMode else {
return
}

@@ -15,13 +15,18 @@ public struct Folder: Equatable {
public var title: String
public var color: String
public let count: Int
public var shared: Bool = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public var shared: Bool = false
public var isShared: Bool = false


//TODO: - 추후 구현
// button.addTarget(self, action: #selector(didTapSubscribeButton), for: .touchUpInside)
button.addTarget(self, action: #selector(didTapSubscribeButton), for: .touchUpInside)
Copy link
Contributor

Choose a reason for hiding this comment

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

아무리 생각해봐도 addTarget으로 액션 연결하는건 너무 불필요한 코드가 많이 쓰이는것 같아ㅠ
UIControl에 publisher를 익스텐션해서 제공하거나 해서 RxSwift나 ReactorKit 쓰던 방식처럼 반응형으로 시퀀스 기반으로 이벤트 전달하면 훨씬 보기도 깔끔할텐데 아쉽다ㅠㅠ

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 290 to 296
viewModel.action(.didTapStopSharingButton(folder.id))
viewMode = .ownerFirstEnter
shareButtonStackView.setupStackView(viewMode: viewMode)
case .subscriber:
viewModel.action(.didTapStopSubscriptionButton(folder.id))
viewMode = .subscriberFirstEnter
shareButtonStackView.setupStackView(viewMode: viewMode)
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
Member Author

Choose a reason for hiding this comment

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

그 형태로 짜놓은거긴 한데 네이밍 때문에 헷갈린 것 같다ㅜㅜ
네이밍 수정해봤는데 피드백 부탁할게!!

@Jeongsuu
Copy link
Contributor

Scheme 타고 들어올때 알 수 있는 페이로드가 FolderID 뿐이니
FolderCoordinator 구현부에 startPush에서 FolderID 값을 받는 형태로 인터페이스를 취하고 내부 라이프사이클에서 API를 찌르는 구조가 되어야 할 것 같아~
리뷰 수정할때 코디네이터 쪽도 같이 작업해주면 좋을듯!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants