-
Notifications
You must be signed in to change notification settings - Fork 5
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
목표시간 초기화 프로세스 개선 #160
목표시간 초기화 프로세스 개선 #160
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.
수고하셨습니다!
너무 늦게 확인해서 죄송합니다..! 🙇🏻♂️🙇🏻♂️
코드리뷰를 하면서 느낀점도 공유드릴께요!
- profile 과 같은 프로젝트 관련 변동사항으로 인한 문제를 해결하기 위한 tuist 적용 고민 필요성
- 코드컨벤션 논의 및 Swift Lint 필요성
@@ -2686,6 +2705,8 @@ | |||
MARKETING_VERSION = 7.17.2; | |||
PRODUCT_BUNDLE_IDENTIFIER = "$(APP_BUNDLE_ID)"; | |||
PRODUCT_NAME = "$(APP_NAME)"; | |||
PROVISIONING_PROFILE_SPECIFIER = ""; |
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.
P5 (의견)
profile이 git 변경사항으로 잡히는군요 ㅠㅠ
tuist 적용이 필요할지 고민되네여..
@@ -15,7 +15,14 @@ final class RecordsManager { | |||
var recordTimes = RecordTimes() | |||
var currentDaily = Daily() | |||
var currentTask: Task? | |||
var showWarningOfRecordDate: Bool = false | |||
//FIXME: 회의 후 결정 및 변경 | |||
private let resetHour = 6 |
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.
P4 (참고)
private let resetHour: Int = 6
var isDateChanged: Bool { | ||
let today = Date() | ||
let compareDay = currentDaily.day.nextDay.setTime(hour: resetHour) | ||
return today >= compareDay | ||
} |
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.
P4 (참고)
var isDateChanged: Bool {
let now = Date()
let compareTime: Date = self.curentDaily.day.nextDay.setTime(hour: resetHour)
return now >= compareTime
}
@@ -16,7 +16,8 @@ final class DailysUseCase: DailysUseCaseInterface { | |||
} | |||
|
|||
func uploadDailys(dailys: [Daily], completion: @escaping (Result<Bool, NetworkError>) -> Void) { | |||
self.repository.upload(dailys: dailys) { result in | |||
let newDailys = dailys.filter { $0.totalTime > 0} |
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.
P4 (참고)
let newDailys: [Daily] = dailys.filter { $0.totalTime > 0 }
@@ -321,7 +314,7 @@ extension StopwatchVC { | |||
self.taskButton.titleLabel?.font = Typographys.uifont(.semibold_4, size: 18) | |||
} | |||
private func configureRendering() { | |||
self.settingBT.setImage(UIImage.init(systemName: "calendar.badge.plus")?.withRenderingMode(.alwaysTemplate), for: .normal) | |||
self.settingBT.setImage(.init(named: "calendar")?.withRenderingMode(.alwaysTemplate), for: .normal) |
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.
P5 (의견)
Image의 경우 한곳에서 관리되면 어떨까요?
티티 코드가 가물가물하지만...ㅎㅎ
Core / Design System / Icons 내 Icons 파일에 추가할 예정이였는지, 없어진건진 모르겠지만
이런식으로 관리되면 좋을 것 같아요!
struct Icons {
static var calendar: UIImage? = { .init(named: "calendar")?.withRenderingMode(.alwaysTemplate) }
}
@@ -67,7 +67,9 @@ final class StopwatchVM { | |||
} | |||
|
|||
private func checkRecordDate() { | |||
self.warningNewDate = RecordsManager.shared.showWarningOfRecordDate | |||
if RecordsManager.shared.isDateChanged { | |||
newRecord() |
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.
P4 (참고)
self 추가가 가능한 경우 모두 반영되면 빌드 속도, 그리고 해당 코드가 어디에 속하는지를 빠르게 알 수 있을 것 같아요!
newRecord 메소드 명칭도 createNewRecord 또는 createNewDaily 식으로 변경되면 좋을 것 같습니다!
self.newRecord()
} else { | ||
self.checkRecordDate() |
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.
P1 (꼭 확인)
checkRecordDate 함수 내에서 호출된 newRecord 함수가 동기적으로 이뤄지는지 확인이 필요할 것 같아요!
Daily 수정 및 저장이 이뤄진 이후에 updateAnimationSetting 메소드가 호출되는지 확인 부탁드립니다!
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.
리뷰 요청
개요
변경사항
DailyUseCase
->uploadDailys()
Timer & Stopwatch 변경사항
기록 동기화 버그 개선
DailyUseCase
에서uploadDailys()
할 때 비어있는 Daily 때문에 정상 동작 하지 않았던 버그를 해결했어요QA 프로세스
💡
RecordsManager
의resetHour
를 조작하면 가까운 시간에 테스트 할 수 있어요!이때, 전 날의 날짜에 해당 기록 시간이 모두 기록되었는지 확인해요
이때, 다음날로 기록이 되었는지 확인해요