-
Notifications
You must be signed in to change notification settings - Fork 16
[Feat] #493 - 신규 홈뷰 - 화면 전환 구현 #494
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
Conversation
|
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.
너무 고생하셨습니다!! 홈뷰 다 했다 .. . 아자아자!!
+) 참고사항 부분은 자고 일어나서 다시 생각해보겠습니다 ! ! !
@@ -20,17 +20,15 @@ public extension UILabel { | |||
} | |||
} | |||
|
|||
func setLineSpacingWithChaining(lineSpacing: CGFloat) -> UILabel { | |||
let label = self | |||
func setLineSpacingWithChaining(lineSpacing: CGFloat) { |
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.
함수명을 보니 해당 함수는 메소드 체이닝을 위해 반환값을 만들어둔 것 같아요. 반환값이 필요없다면 같은 역할을 하는 setLineSpacing
함수를 사용하는 것이 좋을 듯 합니다!
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.
ㅋ ㅋ 원래대로 돌려놓을게요 오 오 ..
userService.getUserMainInfo() | ||
.mapError { error -> MainError in | ||
guard let error = error as? APIError else { | ||
return MainError.networkError(message: "Moya 에러") |
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.
제가 생각한 대안책은 두 가지가 있습니다.
1️⃣ requestObjectWithNetworkErrorInCombine
의 Error를 모두 MoyaError로 감싸기
APIError가 아닌 부분은 모두 MoyaError로 처리할 수 있어 에러 처리가 명확해져요.
// requestObjectWithNetworkErrorInCombine
// AS-IS
promise(.failure(error))
// TO-BE
promise(.failure(MoyaError.objectMapping(error, value)))
// HomeRepository
...
guard let error as? APIError else {
return MainError.networkError(message: "MoyaError \(error.localizedDescription)")
}
실제 MoyaError로 처리되는지 테스트하기 위해 MoyaError 코드를 수정해보았어요.
단점은 적절한 MoyaError를 지정해주어야한다는 것인데요, 번거로움을 줄이기 위해 error.localizedDescription
자체를 반환하는 MoyaError.underlying으로 모두 감싸는 것이 더 좋아보여요.
2️⃣ error.localizedDescription
를 반환
APIError가 아닌 경우 에러 타입에 관계없이 error.localizedDescription을 반환합니다.
개인적으로는 default 부분에서 error.localizedDescription을 반환하고, 해당 부분에서는 MoyaError로 감싸는 방식이 더 명확하다고 생각해요!
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.
2번 방식으로 수정해두었어용
@@ -126,7 +128,7 @@ extension CalendarCardCVC { | |||
extension CalendarCardCVC { | |||
func configureCell(model: HomePresentationModel.RecentSchedule, | |||
userType: UserType) { | |||
self.dateLabel.text = model.date | |||
self.dateLabel.text = setDateFormat(date: model.date, to: "MM.dd") |
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.
dateFormatter
를 매번 생성하는 것이 성능상 좋지 않다고 알고있는데, 이 부분 주석으로 메모해두고 추후 서버 선생님들께 바꿔 내려달라고 하는건 어떠신가요 ? 저희도 dateFormatter를 재활용하도록 추후 리팩토링 해봅시다 ㅋ.ㅋ
참고링크
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.
아 이거 저번에 dateFormatter 성능 관련해 얘기하면서 ... 서버에 요청하기로 구두로 얘기했던 것 같은데 (그래서 안 고쳐놨었나봐요 ㅋㅋ ) 얘도 서버 쌤들이 바꿔주시고 나면 원래대로 돌려놓겠습니다!
@@ -70,7 +132,7 @@ public final class HomeCoordinator: DefaultCoordinator { | |||
} | |||
|
|||
homeCalendarDetail.vm.onAttendanceButtonTap = { [weak self] in | |||
self?.requestCoordinating?() | |||
self?.requestCoordinating?(.calendar) |
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.
이 부분은 캘린더 상세 뷰에서 출석체크로 넘어가는 부분이라 .attendance
로 바꿔주세요! 감사합니다🙇♀️🙇♀️
+) 스크린샷을 보았을 때 출석을 하는 방법이 행사 부분 클릭 > 캘린더 상세뷰 > 출석
과 홈뷰에서 출석 버튼을 눌렀을 때
로 두 가지 같은데 이 부분이 어색하게 느껴지네욥,, (재현님 의견도 궁금해요! 같은 의견이라면 ,, 홈 뷰 개편이 끝난 후 기획측에 건의해보겠습니다ㅎㅎ)
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.
좋아요! QA하는 날 말씀드려보아요~!
struct UserInfo: Identifiable, Hashable { | ||
let id = UUID() | ||
|
||
let history: [Int] | ||
|
||
init(history: [Int]) { | ||
self.history = history |
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.
아 이거 쓰는 거여요! DashBoard에서 description이랑 historyList랑 같이 받아주고 있거든유
struct DashBoard: Identifiable, Hashable {
let id = UUID()
let description: String
let historyList: [Int]?
init(description: String, userInfo: UserMainInfoModel? = nil) {
self.description = description
self.historyList = userInfo?.historyList
}
}
struct UserInfo: Identifiable, Hashable {
let id = UUID()
let history: [Int]
init(history: [Int]) {
self.history = history
}
}
// MARK: - toPresentation
extension HomeDescriptionModel {
func toPresentation(userInfo: UserMainInfoModel) -> HomePresentationModel.DashBoard {
return HomePresentationModel.DashBoard(
description: self.description,
userInfo: userInfo
)
}
}
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.
아하 !! 그러면 DashBoard에서 init할 때 UserMainInfoModel 대신 UserInfo를 사용해야하는거 아닌가요?
그리고 DashBoard와 UserInfo를 나누는 이유도 궁금합니다!
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.
아. 하 그러게요? 이거 UserInfo 없어도 되는 거 맞아요
아 이게.. 제가 DashBoard 하나로 구조체를 묶으려다가
input.viewDidLoad
.handleEvents(receiveOutput: { _ in
output.isLoading.send(true)
})
.flatMap { _ in
self.useCase.getUserInfo()
}
.compactMap { $0 }
.flatMap { userInfo in
Publishers.Zip3(
self.useCase.getHomeDescription().map { $0.toPresentation(userInfo: userInfo) },
위처럼 userInfo 값을 먼저 받은 후에, description이랑 같이 DashBoard로 합쳐주고 있거든요...
이거 뭔가 어색한 것 같기도 한데 어떠세요? 둘이 구조체 각각 나누는 방법도 있는데 뭔가 DashBoard 하나로 묶는 게 화면상에서는 자연스러운 것 같아서요
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.
아하 ~ ! 그러면 혹시 userInfo에 있는 history로 DashBoard를 구성하는건 어떤가요?
// ViewModel
input.viewDidLoad
.handleEvents(receiveOutput: { _ in
output.isLoading.send(true)
})
.flatMap { _ in
self.useCase.getUserInfo()
}
.compactMap { $0 }
.flatMap { userInfo in
Publishers.Zip3(
// 수정 부분
self.useCase.getHomeDescription().map { $0.toPresentation(history: userInfo.historyList) },
...
// HomePresentationModel
struct DashBoard: Identifiable, Hashable {
let id = UUID()
let description: String
let historyList: [Int]?
init(description: String, history: [Int]) {
self.description = description
self.historyList = history
}
}
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.
아하~~~ 좋아요 !!! 반영해두겠습니다
var cancelBag = CancelBag() | ||
private var cellTapped = PassthroughSubject<HomeForMemberItem, Never>() | ||
var attendanceButtonTapped = PassthroughSubject<Void, Never>() |
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.
var cancelBag = CancelBag() | |
private var cellTapped = PassthroughSubject<HomeForMemberItem, Never>() | |
var attendanceButtonTapped = PassthroughSubject<Void, Never>() | |
private(set) var cancelBag = CancelBag() | |
private var cellTapped = PassthroughSubject<HomeForMemberItem, Never>() | |
private(set) var attendanceButtonTapped = PassthroughSubject<Void, Never>() |
} | ||
|
||
private func applySnapshot(with appService: [HomePresentationModel.AppService]) { | ||
var snapshot = NSDiffableDataSourceSnapshot<HomeForVisitorSectionLayoutKind, HomeForVisitorItem>() | ||
|
||
snapshot.appendSections(HomeForVisitorSectionLayoutKind.allCases) | ||
|
||
snapshot.appendItems([.description(HomePresentationModel.Description(description: ""))], toSection: .dashBoard) | ||
snapshot.appendItems([.dashBoard(HomePresentationModel.DashBoard(description: ""))], toSection: .dashBoard) |
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.
생성자에서 description default값으로 "" 넣어주는건 어떠신가요?
case .calendar: | ||
self?.runAttendanceFlow() |
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.
아하 여기서 출석뷰로 이동해주고 있군요!! calendar를 attendance로 통일하는건 어떠신가요? destination의 의미상 calendar는 캘린더 상세뷰로 이동한다고 생각할 수 있을 것 같아요.
@@ -14,18 +14,20 @@ import DSKit | |||
|
|||
final class CalendarCardCVC: UICollectionViewCell { | |||
|
|||
// MARK: - Properties | |||
|
|||
lazy var attendanceButtonTap = attendanceButton.publisher(for: .touchUpInside) |
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.
lazy var attendanceButtonTap = attendanceButton.publisher(for: .touchUpInside) | |
private(set) lazy var attendanceButtonTap = attendanceButton.publisher(for: .touchUpInside) |
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.
리뷰 감사해~용가리 ㅋ
@@ -20,17 +20,15 @@ public extension UILabel { | |||
} | |||
} | |||
|
|||
func setLineSpacingWithChaining(lineSpacing: CGFloat) -> UILabel { | |||
let label = self | |||
func setLineSpacingWithChaining(lineSpacing: CGFloat) { |
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.
ㅋ ㅋ 원래대로 돌려놓을게요 오 오 ..
@@ -126,7 +128,7 @@ extension CalendarCardCVC { | |||
extension CalendarCardCVC { | |||
func configureCell(model: HomePresentationModel.RecentSchedule, | |||
userType: UserType) { | |||
self.dateLabel.text = model.date | |||
self.dateLabel.text = setDateFormat(date: model.date, to: "MM.dd") |
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.
아 이거 저번에 dateFormatter 성능 관련해 얘기하면서 ... 서버에 요청하기로 구두로 얘기했던 것 같은데 (그래서 안 고쳐놨었나봐요 ㅋㅋ ) 얘도 서버 쌤들이 바꿔주시고 나면 원래대로 돌려놓겠습니다!
userService.getUserMainInfo() | ||
.mapError { error -> MainError in | ||
guard let error = error as? APIError else { | ||
return MainError.networkError(message: "Moya 에러") |
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.
앗 말씀해주신대로 반영하고 다시 리뷰달겠습니다! 감사합니다
@@ -70,7 +132,7 @@ public final class HomeCoordinator: DefaultCoordinator { | |||
} | |||
|
|||
homeCalendarDetail.vm.onAttendanceButtonTap = { [weak self] in | |||
self?.requestCoordinating?() | |||
self?.requestCoordinating?(.calendar) |
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.
아하 이거 제가 약간 헷갈렸던 것 같아요! 잘 수정해놓겠습니다
스크린샷을 보았을 때 출석을 하는 방법이 행사 부분 클릭 > 캘린더 상세뷰 > 출석과 홈뷰에서 출석 버튼을 눌렀을 때로 두 가지 같은데 이 부분이 어색하게 느껴지네욥,,
저도 어색하다고 생각하기는 했는데.. 아무래도 홈화면에 드러나는 출석 버튼이 작아서, 캘린더 상세뷰 안에두 넣어두지 않았을까.. 하는 개인적 추측을 해봅니다.. 그치만 윤서 님 말씀대로 기획에 건의해보는 것은 완전 찬성! 합니다
@yungu0010 코리 반영해뒀으니 확인 부탁드려요옹 |
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.
우와아ㅏ아아 고생하셨습니다🫶🫶🫶🫶
🌴 PR 요약
신규 홈뷰(활동/비회원 전용)에 필요한 화면 전환을 구현했습니다.
🌱 작업한 브랜치
🌱 PR Point
신규 홈뷰(활동/비회원 전용)에 포함된 화면 전환
기타 작업 내용
📌 참고 사항
HomePresentationModel
내 중첩된 모델들의 id 값을 어떻게 줘야할지 짧게 고민해봤는데, 데이터 자체에 고유성을 보장할 수 있는 필드가 있는 경우, 그 값으로 지정해줬는데요 (id, memberId, deeplink 등) 괜찮은지 의견 부탁드립니다.그리고, 이전 이슈에서 반영하지 못했던 코드 리뷰 반영해뒀습니다! 확인 부탁드려요 🙇🏼
최대한 피그마 확인하면서 놓친 분기 없는지 ... 주의하면서 구현하긴 했는데,
(그래도 고쳐야 될 부분이 좀 있는 것 같아요 🥹)
혹시 제가 놓친 부분 있다면 피드백 부탁드립니다.
다음주 배포까지.. 아자아자
📸 스크린샷
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-02-14.at.23.11.05.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-02-14.at.23.12.00.mp4
📮 관련 이슈