-
Notifications
You must be signed in to change notification settings - Fork 1
Feat [#135] Maintenance 시스템 점검 구현 #136
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
| "filename" : "maintenance.svg", | ||
| "idiom" : "universal", | ||
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } |
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.
single scale로 수정해주세용~
| let message = self.remoteConfig["downtime_message_iOS"].stringValue ?? "none" | ||
|
|
||
| if message != "none" { |
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.
여기서 그냥 빈값"" 말고 "none"으로 한 이유가 있나요?
remoteConfig에서 값이 없으면 "none"으로 온다든지?
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.
Remote Config에 저희가 직접
값을 입력하여 게시하는 구조입니다!
불명확한 값보다 none과 같은 명확한 값을
넣는게 확실하다는 생각이었습니다..
어떻게 생각하시는지요?🤔
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.
아 그러면 Remote Config에서 값이 없을 땐 "none"으로 오게 설정할 수 있다는 뜻이죠??
뭐 사실 상관없을 거 같긴 한데.. 디폴트값을 따로 줄 바에는(이것도 nil 처리하는 거니까) 차라리 옵셔널 타입으로 써서 if let 언래핑 방식은 어떤가 싶습니다
| init() { | ||
| super.init(frame: .zero) | ||
| } | ||
|
|
||
| required init?(coder: NSCoder) { | ||
| fatalError("init(coder:) has not been implemented") | ||
| } |
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.
init에서 추가 설정할 거 없으면 삭제해도 될 거 같슴다
|
|
||
| containerView.snp.makeConstraints { | ||
| $0.center.equalToSuperview() | ||
| $0.leading.trailing.equalToSuperview().inset(ScreenUtils.getWidth(20)) |
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.
여기 inset 수치가 24인데 확인 부탁드려욤
| AppVersionManager.shared.checkForUpdateAndProceed { [weak self] shouldProceed in | ||
| guard shouldProceed else { return } | ||
| self?.proceedToNextViewController() | ||
|
|
||
| AppVersionManager.shared.checkForDowntimeAndProceed { canProceed in | ||
| guard canProceed else { return } | ||
| self?.proceedToNextViewController() | ||
| } | ||
| } |
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.
shouldProceed, canProceed 네이밍이 비슷해서 좀 더 구체적으로 쓰면 가독성이 좋아질 것 같아요
예를들면 bool값이니까 업데이트 필요 여부, 다운타임 여부로 네이밍하는 건 어때용?
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.
checkForUpdateAndProceed가 false일 경우, checkForDowntimeAndProceed를 할 필요도 없는 건가요?
=> 버전 업데이트가 다운타임보다 우선순위가 높은 로직이 맞는건지?
만약 각각 독립적인 로직이라면 비동기로 처리하는 게 더 빠를 거 같아서요! 그럼 너무 파이어베이스 RemoteConfig 호출이 많아져서 그런가요?
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.
그렇다면 이대로 진행해도 좋겠네요!! 🙂
| $0.attributedText = UIFont.pretendardString( | ||
| text: "보다 안정적인 클로디 서비스를 위해\n시스템 점검 중이에요. 곧 다시 만나요!", | ||
| style: .body3_medium | ||
| ) |
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.
titleLabel에 행간 설정 파라미터 lineHeightMultiple도 1.5로 설정해야 할듯!
Nya128
left a comment
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.
확인했습니다! 고생하셨어요~👍
🍀 작업 내용
Maintenance 라는 단어를 많이 사용하더라고요! 그래서 시스템 점검 창을 다음과 같이 정하였습니다.
동작 흐름
AppVersionManager에서 firebase로 부터 값을 받아와
"none" 이 아니면
다운 타임이라고 판단하여, MaintenanceVC로 넘깁니다.
🚀 PR Point
📸 스크린샷
✚ 코드 리뷰 반영 사항
✅ CheckList
🔗 Issue
Resolved #135