-
Notifications
You must be signed in to change notification settings - Fork 15
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] #32 - SignUpView alert 로직 변경 및 SignUpVC UI 구현 #33
[Feat] #32 - SignUpView alert 로직 변경 및 SignUpVC UI 구현 #33
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.
고생하셨습니당 👏🏻
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.passwordCheckTextFieldView.changeAlertLabelText(alertText) | ||
if !alertText.isEmpty { | ||
self.passwordCheckTextFieldView.changeAlertLabelTextColor(toWarning: true) | ||
if alertText == I18N.SignUp.invalidPasswordForm { | ||
self.passwordTextFieldView.alertType = .invalidInput(text: "") | ||
} else { | ||
self.passwordTextFieldView.alertType = .validInput(text: "") | ||
self.passwordCheckTextFieldView.alertType = .invalidInput(text: alertText) | ||
} | ||
} else { | ||
self.passwordTextFieldView.alertType = .validInput(text: "") | ||
self.passwordCheckTextFieldView.alertType = .validInput(text: "") | ||
} |
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.
지금 하나의 alertType을 가지고 passwordTextField와 checkTextField 모두에 대한 데이터를 바인딩하려다 보니 분기가 복잡해지고 가독성도 떨어지는 것 같습니다.
각각의 텍필에 대한 데이터를 따로 만들어주고 바인딩하는 방식도 고려해 볼 수 있을 것 같아요...!
예를 들어 SignUpFormValidateResult의 extension에 메서드를 따로 만들어서 프로퍼티 각각을 참조하게 만드는 것입니다!
우선 true에 대한 입력만 받아야하는지는 모르겠지만
let needWarning = !alertText.isEmpty
self.passwordCheckTextFieldView.changeAlertLabelTextColor(toWarning: needWarning)
이런 식으로 할 수 있을 것 같고
.map { $0.convertToTextFieldAlertType() }
의 경우에 alerType으로 변환해주고 있는데, 여기서도 alertType 자체가 아니라 text를 통해 분기처리를 하고 있어 불필요한 부분인 것 같습니다.
위의 부분을 매핑을 사용하지 않은 것으로 가정하면 아래와 같이 연산 프로퍼티를 따로 만들어 처리하는 것도 하나의 방법이라 생각합니다...!
self.passwordTextFieldView.alertType = signUpFormValidateResult.passwordAlertType
self.passwordCheckTextFieldView.alertType = signUpFormValidateResult.passwordCheckAlertType
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개고 alertTextLabel은 한 개라 입력 validation 이후에 분기 처리가 많이 필요했습니다. 따라서 심플하게 생각해서 CustomTextField 클래스에 alertDelegate라는 변수를 만들고 해당 값이 존재한다면 alert 기능을 delegate인 텍스트필드에 위임하도록 했습니다.
즉, 첫번째 패스워드 입력 뷰는 alertText를 보여주는 기능을 두번째 패스워드 입력 뷰에 위임하여 VC에서는 뷰의 delegate만 지정하고 첫번째 패스워드뷰에 alert를 보내면 알아서 내부적으로 두 번째 패스워드 뷰에 alert가 보이도록 했습니다.
이 방식으로 바꾸니까 VC에서는 닉네임, 이메일 텍스트필드처럼 패스워드 텍스트필드를 동일하게 다룰 수 있어서 가독성도 좋아진 것 같습니다. 한번 확인하고 피드백 주실 수 있나요!? 😃
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.
수고하셨습니다 .. 여기도 분기처리가 쉽지 않네요 .. 🫠👍
@@ -79,26 +86,26 @@ extension SignUpViewModel { | |||
useCase.isNicknameValid.sink { event in | |||
print("SignUpViewModel - completion: \(event)") | |||
} receiveValue: { isNicknameValid in | |||
isNicknameValid ? output.nicknameAlert.send("") : output.nicknameAlert.send(I18N.SignUp.duplicatedNickname) | |||
isNicknameValid ? output.nicknameAlert.send(.valid(text: I18N.SignUp.validNickname)) : output.nicknameAlert.send(.invalid(text: I18N.SignUp.duplicatedNickname)) |
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.
취향에 따라 가독성 좋은게 다를 것 같긴 한데,
요렇게 쓰는 것도 중복되는 코드 줄이는 면에서 나쁘지 않을 것 같아서 적어봅니다 !
output.nicknameAlert.send(isNicknameValid ? .valid(text: I18N.SignUp.validNickname) : .invalid(text: I18N.SignUp.duplicatedNickname))
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.
리뷰 반영하고 로직 고민하느라 고생하셨습니다!!
public func setAlertDelegate(_ textView: CustomTextFieldView) -> Self { | ||
self.alertDelegate = textView | ||
return self | ||
} | ||
|
||
/// 경고 문구 라벨의 text 설정 | ||
public func changeAlertLabelText(_ alertText: String) { | ||
if let alertDelegate = alertDelegate { | ||
alertDelegate.changeAlertLabelText(alertText) | ||
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.
새로운 방식으로 하니 더 깔끔해지네요!! 비대한 함수의 기능이 잘 분리된 것 같아요👍 👍 평소에 하듯이 alertDelegate를 Protocol로 구현해주면 확장성도 좋아질 것 같아요~
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
📌 참고 사항
📸 스크린샷
📮 관련 이슈