-
Notifications
You must be signed in to change notification settings - Fork 2
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
[refactor] 약속 상세(PagePromise, PromiseInfo, ReadyStatus, Tardy) ViewController와 ViewModel 재구현 #390
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.
LGTM🫶🏻💗
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.
LGTM👍
@@ -30,17 +30,13 @@ class ObservablePattern<T> { | |||
|
|||
func bind(_ listener: @escaping (T) -> Void) { | |||
listeners.append(listener) | |||
|
|||
listener(value) |
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.
고생하셨습니다.
Observable Pattern의 코드 구조를 변경하다 보니, 가장 연관되는 약속 상세 화면의 경우, 코드를 다시 구현하신거나 다름 없이 됐네요.
그 만큼 앱의 핵심 기능이 담긴 곳이다 보니 더욱 그런 것 같습니다.
힘드신 것은 너무나 잘 알고 있지만, QA를 반영하는 PR인 만큼 어떤 코드가 어떤 QA를 반영하는지 등 PR을 작성하는 데에 있어 좀 더 공을 들이는 것이 좋아 보입니다.
결국 남에게 자신의 코드를 보여주고, 여러 의견을 주고 받는 이 자리에서 코드를 작성한 본인보다 코드를 더 잘 이해할 수는 없기 때문입니다.
원활한 소통을 위해서는 코드에 대한 이해도를 나와 상대방이 최대한 동일 선상에서 맞춘 다음 소통이 이루어져야 효과적으로 의견 공유가 가능하다고 생각하거든요.
저 같은 경우는, PR을 작성할 때 짧게는 20-30분 길게는 60분 정도 작성하는 편입니다.
이런 수준까지를 바라는 것은 아니지만, 앞으로도 도움이 될 것 같아 제 지난 부트캠프 PR 하나를 링크로 달아두겠습니다. (tasty-code/ios-weather-forecast#39)
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.
하하.. 다시 한 번 사과를 드립니다.
extension CreateMeetingViewController: UITextFieldDelegate { | ||
func textFieldDidBeginEditing(_ textField: UITextField) { | ||
textField.layer.borderColor = UIColor.maincolor.cgColor | ||
} | ||
|
||
func textFieldDidEndEditing(_ textField: UITextField) { | ||
rootView.characterLabel.textColor = .gray3 | ||
rootView.nameTextField.layer.borderColor = UIColor.gray3.cgColor | ||
} | ||
} |
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 반영이겠죠?
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.
넵 4차 QA 반영입니다!
if success { | ||
let viewController = AddPromiseCompleteViewController(promiseID: self.viewModel.promiseID) |
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.
self
대신 owner
사용 부탁드려요.
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.
반영했습니다!
) | ||
private lazy var promiseSegmentedControl = PagePromiseSegmentedControl(items: ["약속 정보", "준비 현황", "지각 꾸물이"]) |
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
로 선언되어야 할 필요가 있나요?
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.
UIButton도 lazy var을 사용하는데 비슷한 느낌이라고 생각했습니다. 복잡한 컴포넌트라서 lazy로 사용하는게 조금 더 성능을 개선시킬 수 있지 않나 ... 생각해서 lazy로 선언했습니다.
promiseTardyViewController.viewModel.updatePromiseCompletion { | ||
DispatchQueue.main.async { | ||
viewModel.isFinishSuccess.bindOnMain(with: self) { owner, isSuccess in | ||
guard let isSuccess = isSuccess else { return } |
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.
guard let isSuccess else { return }
도 가능합니다.
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.
수정했습니다!
case 1...: | ||
owner.rootView.dDayLabel.setText("D-\(dDay)", style: .body05, color: .gray5) | ||
case 0: | ||
owner.rootView.dDayLabel.setText("D-DAY", style: .body05, color: .mainorange) | ||
case ..<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.
👍
viewModel.isPastDue.bindOnMain(with: self) { owner, _ in | ||
owner.rootView.editButton.isHidden = owner.viewModel.isEditButtonHidden() | ||
} |
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.
저 같은 경우, UIButton의 isHidden
이 프로퍼티로 구현되어 있기 때문에, viewModel
의 isEditButtonHidden
의 경우도 계산 속성으로 구현하는 편입니다.
$0.tardyEmptyView.isHidden = true | ||
$0.noTardyView.isHidden = true | ||
|
||
owner.rootView.tardyCollectionView.reloadData() |
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.
do
블럭 내부 코드의 일관성을 위해서, $0.tardyCollectionView.reloadData()
로 수정되는 것이 좋아보입니다.
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.
수정했습니다!
|
||
owner.rootView.tardyCollectionView.reloadData() |
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.
do
블럭 내부 코드의 일관성을 위해서, $0.tardyCollectionView.reloadData()
로 수정되는 것이 좋아보입니다.
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.
수정했습니다!
placeholder: UIImage.imgProfile | ||
) | ||
cell.nameLabel.setText(tardyName, style: .body06, color: .gray6) | ||
cell.profileImageView.kf.setImage(with: URL(string: viewModel.tardyList.value[indexPath.row].profileImageURL ?? ""), placeholder: UIImage.imgProfile) |
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 시간이 얼마 안남아서 그냥 올리려고 했는데 ... 반성하게 되네요. 좋은 지적 감사드립니다! |
🔗 연결된 이슈
📄 작업 내용
📸 스크린샷