-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Design] 사용자 등록 뷰 디자인 #60
Conversation
about registerviewcontroller to homeviewcontroller
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.
윤철님 수고하셨습니다 .ᐟ.ᐟ 👍 👍
Combine을 학습하시는데 시간이 많이 걸리신거였군요.. 정확히 어떤 이유에서인지는 모르겠지만 정말 딱 화면을 나타내는 View와 ViewController를 분리하려고 하셨던 것 같습니다. 보통 이렇게 View를 따로 만드는 경우는 제가 알기론 같은 뷰가 다른 곳에서도 재활용되는 경우라고 알고있어요 ! 그 화면에서만 사용될 뷰인 경우에는 굳이 ViewController와 분리하지 않을 거에용..
물론 View가 재활용되는 경우에만 꼭 분리해야된다라는 이야기는 아닙니다 ! ViewController가 너무 비대해져 가독성이 떨어지는 것을 방지하기 위해서도? 분리할 수 있다고 생각합니다. 하지만 View와 ViewController를 분리한다면 View에는 정말 딱 그 화면을 그리는 프로퍼티와 레이아웃 관련 코드만 들어가야한다고 생각합니다 ! View가 textField의 값 변경을 감지하고 그걸 어디론가 send한다면 ViewController가 하는일이.. 무엇인가요..??
현재 윤철님이 짜주신 코드도 좋지만 한 번 생각해보시면 좋을 것 같아 이렇게 부족한 지식을...모아모아 주저리주저리 써봤숩니당.. ㅎㅅㅎ 제가 너무 정형화된? 생각에 갇혀있는 것일 수도 있긴합니다 .ᐟ.ᐟ 하지만 제 생각엔 이렇게 코드를 짜게된 이유? 를 잘 모르겠어서 윤철님의 해당 코드에 대한 설명을 한 번 들어보고싶네용
다시한번 수고 많으셨습니다 .ᐟ.ᐟ
+) 추가로 레이아웃 구현은 feature에 해당되니 Label에 Feature도 추가해주시면 좋을 것 같습니당 .ᐟ.ᐟ
}() | ||
|
||
// MARK: - Initializers | ||
init() { super.init(frame: .zero) } |
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.
P2: 해당 initializer는 없어도 되지 않나욥?? 어떨때 사용되는 이니셜라이저인가요??
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.
사용하려고 만들었다가 사용하지 않는 코드가 됐는데 제거를 못했네요
제거 했습니다 ~
} | ||
|
||
required init?(coder: NSCoder) { | ||
fatalError("error: mhregisterview requires init(coder:)") |
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.
P2: 저희 컨벤션에 따라서 required init 함수 내부도 위 오버라이딩 한 이니셜 라이저와 똑같게 구현해줘야할 것 같습니다 !
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 func addTouchEventToRegisterButton(_ button: UIButton) { | ||
let uiAction = UIAction { [weak self] _ in |
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: 해당 함수 내부에서만 사용되는 지역변수이긴 하지만 정확히 어떤 동작을 하는지 변수명만 봤을때 알 수 없을 것 같아요.. 네이밍을 다시 지어보면 좋을 것 같습니다 !
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.
RegisterButton에 터치 이벤트를 준다는 의미인데 혹시 추천해주실만한게 있을까요?
let userDefaults = UserDefaults.standard | ||
userDefaults.set(houseName, forKey: Constant.houseNameUserDefaultKey) |
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: UserDefault에 값을 저장하는 로직은 ViewModel에 들어가는게 더 적합해보입니다 !
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.
오호! Register 로직에서 ViewModel을 생성하지 않아서 Viewcontroller로 로직을 옮겼는데 어떨까요?
let uiAction = UIAction { [weak self] _ in | ||
guard let houseName = self?.registerTextField.text, !houseName.isEmpty else { return } | ||
|
||
if self?.checkNameValidity(houseName) ?? false { |
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.
P3: self를 guard문을 활용해 옵셔널 언래핑을 해주면 조금 더 깔끔하게? 사용할 수 있을 것 같습니당. 해당 클로저 내부에는 비동기 코드가 따로 존재하지 않으니 self 캡쳐를 guard문으로 해줘도 되지 않을까..? 라는 생각입니당
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 houseName = self?.registerTextField.text, !houseName.isEmpty else { return } | ||
|
||
if self?.checkNameValidity(houseName) ?? false { | ||
MHRegisterView.buttonSubject.send(true) |
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: View끼리 바인딩을하고 view에서 특정 값을 능동적으로 방출하는 방식이 어색해보입니다.. View끼리 데이터를 주고받을 떄에는 메서드를 통해서도 충분히 가능하지 않을까요??
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 UITextField { | ||
enum Tag { | ||
static let register = 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.
P1: 해당 화면에선 UITextField가 하나밖에 존재하지 않는데 Tag를 사용해야하는 이유가 무엇인지 궁금합니다 !
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.
삭제 완!
} | ||
|
||
// MARK: - Configure | ||
private func configureAddSubviewAndConstraints() { |
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: 해당 함수 내부에 Constraints를 잡아주거나 addSubView를 해주는 동작 말고도 layer를 설정하는 등의 부가적인 동작들이 존재하는 것 같습니다. 함수를 분리해보는게 좋아보입니다.
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.
윤철님 고생하셨습니다! 기차 안이라 시간이 좀 걸렸네요 😅 리뷰 확인하시고 의문드는 점 있으면 언제든 물어보셔도 됩니당!
|
||
let textField = UITextField() | ||
textField.font = registerFont | ||
textField.borderStyle = .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.
P3. border가 없으면 설정 안하는 것과는 차이가 없을까요?
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.
default가 none이네요? 삭제했습니다 ~
trailing: self.trailingAnchor, | ||
constantTrailing: 40, | ||
|
||
height: MHRegisterView.registerTextFieldFontSize * 4 |
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.
P2. 이렇게 고정된 폰트 사이즈로 높이를 잡는 것이 일반적인지 살짝 의문이 드네용... 만약 사용자가 글씨 보기를 키우면 어떻게 보이나요? IntrinsicSize 대신 폰트사이즈 계산으로 정하신 이유가 궁금합니다!
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 houseName = self?.registerTextField.text, !houseName.isEmpty else { return } | ||
|
||
if self?.checkNameValidity(houseName) ?? false { |
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.
P2. 개인적으로 registerTextField의 검증로직이 action과 메서드에서 두차례 수행하는 느낌이 듭니다.
checkNameValidity를 아래의 느낌으로 변경해보는 것은 어떨까요?
private func checkNameValidity() -> Bool {
guard let length = registerTextField.text?.count else { return false }
return length < 11
}
그리고 여기 코드는 이렇게 하면될듯요!
if self?.checkNameValidity() == true { 코드들...
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.
이것도 수정했습니다 ~~
static let textfieldSubject = PassthroughSubject<String, Never>() | ||
static let buttonSubject = PassthroughSubject<Bool, 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.
P3. 제가 Combine을 잘 몰라서 그러는데 원래 static으로 구독을 관리하나요?
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
엇 저도 Static 사용하지 않는 걸로 알고 있습니다 !
static을 사용하신 이유가 어떻게 되나요 ?!
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.
이것도 수정했습니다 ~~ ㅎㅎ
switch textfield.tag { | ||
case UITextField.Tag.register: | ||
if inputText.isEmpty == true { | ||
self.registerButton.isEnabled = false | ||
} else { | ||
self.registerButton.isEnabled = true | ||
} |
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. 영현님이 아래에 달아주시긴 했지만 한가지 덧붙이면, tag는 변경되기 쉬운 값입니다!
따라서 유일성을 보장한다고 확신하기 힘들 것같아요.
이를 통한 ==도 동일성을 보장하기 힘들어지죠.
게다가, 변동이 쉽기에 개발자가 별도로 관리해야하는 노고가 추가됩니다...
textField랑 == 연산자로 비교하면(자동으로 ===실행함) 참조값으로 비교하기 때문에, 동일성을 보장하기 쉬울 것같습니다.
또한, 이 함수에서 textField를 매개변수로 받기보다 registerTextField로 검증해야하지 않을까 싶습니다. 이러면 다른 textField에 같은 tag넣고 조건만 충족되면 registerButton이 눌릴 것같습니다!
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.
고생하셨습니다 윤철님!!
부산가는 기차 안에서의 코드리뷰는 신선하네요
모르는 내용 및 수정되어야 할 부분들 코멘트로 남겨두었습니다 !
static let textfieldSubject = PassthroughSubject<String, Never>() | ||
static let buttonSubject = PassthroughSubject<Bool, 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.
P1
엇 저도 Static 사용하지 않는 걸로 알고 있습니다 !
static을 사용하신 이유가 어떻게 되나요 ?!
registerTextFieldBackground.setAnchor( | ||
top: registerTextLabel.bottomAnchor, | ||
leading: self.leadingAnchor, | ||
constantLeading: 80, | ||
trailing: self.trailingAnchor, | ||
constantTrailing: 40, | ||
height: MHRegisterView.registerTextFieldFontSize * 2 | ||
) | ||
|
||
let registerButtonBackground = UIView() | ||
registerButtonBackground.backgroundColor = .mhSection | ||
registerButtonBackground.layer.cornerRadius = MHRegisterView.registerButtonFontSize + 1 | ||
registerButtonBackground.clipsToBounds = true | ||
registerButtonBackground.addSubview(registerButton) | ||
registerButton.setAnchor( | ||
top: registerButtonBackground.topAnchor, constantTop: 3, | ||
leading: registerButtonBackground.leadingAnchor, constantLeading: 4, | ||
bottom: registerButtonBackground.bottomAnchor, constantBottom: 3, | ||
trailing: registerButtonBackground.trailingAnchor, constantTrailing: 4 | ||
) |
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.
P3
상관없긴 하지만, 104~111번 줄
과 118~123번 줄
에 일관성도 있으면 좋을 것 같습니당
constant를 같은 줄에 쓰는 게 개인적으로 가독성이 더 좋다고 보긴 합니다 !!
(위아래 다른 레이아웃 잡는 부분도 마찬가지입니다 !!)
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.
저도 같은줄에 쓰는게 좋은 거 같아서 전체적으로 수정했습니다!
let userDefaults = UserDefaults.standard | ||
userDefaults.set(houseName, forKey: Constant.houseNameUserDefaultKey) |
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 subscriptions = Set<AnyCancellable>() | ||
|
||
// MARK: - Property | ||
var registerView = MHRegisterView() |
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.
P2
private let으로 가져도 좋을 거 같습니다
var registerView = MHRegisterView() | |
private let registerView = MHRegisterView() |
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.
View, Controller 코드를 합쳤습니다!
let homeViewController = HomeViewController(viewModel: HomeViewModel(houseName: self.username)) | ||
self.navigationController?.pushViewController(homeViewController, animated: false) |
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
(질문입니다)
Register -> Home으로 가는 경우 Register 위에 Home이 쌓이게 될 것 같은데,
이 문제를 해결하기 위해 HomeVC를 새로 Navigation으로 씌우고 그 navigation을 띄워주면 어떨 것 같나용 ??
아니면 이 방식이 자연스러운 걸까요 ?
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?.navigationController?.viewControllers.removeFirst()
해당 코드를 통해 navigation stack에서 제거해주었습니다! 혹시 이러한 방식의 문제점이 있으면 말씀해주세요!
registerView = MHRegisterView( | ||
frame: CGRect.init(x: 0, y: 0, width: view.bounds.width - 50, height: view.bounds.height - 640)) |
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
MHRegisterView를 프로퍼티 선언 부에서는 var registerView = MHRegisterView()
로 생성했는데, 여기서는 frame으로 생성해주시고 계시네요!
영현님 말대로 MHRegisterView에서 init()를 제거하고
프로퍼티 선언 부에는 private let으로 선언하는게 더 가독성이 좋을 것 같습니다
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.
view와 viewcontroller를 합치는 방향으로 수정했습니다 ~~
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.
10점 만점에 10점 드리겠읍니다
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.
정말 너무너무너무 수고 많으셨습니다 .ᐟ.ᐟ 👍👍
살짝 더 고쳐야할 점들만 리뷰 남겨뒀습니당 ~~
|
||
return textLabel | ||
}() | ||
let registerTextField: UITextField = { |
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: registerTextField랑 아래 registerButton의 접근제어자 설정이 빠져있네용 !
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.
접근 제어자 추가했습니다 ~
setup() | ||
bind() | ||
configureAddSubview() | ||
configureConstraints() |
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.
P3: 꼭 그래야하는 건 아닌데 가독성 측면에서 아래 메서드 순서와 함수 호출 순서를 맞추면 좋을 것 같아요 !
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.
순서 맞추는 게 좋을 것 같네요!
attributedString.font = UIFont.ownglyphBerry(size: 12) | ||
attributedString.strokeColor = UIColor.mhTitle | ||
|
||
registerButton.setAttributedTitle(NSAttributedString(attributedString), 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.
registerButton.setAttributedTitle(NSAttributedString(attributedString), for: .normal) | |
registerButton.setAttributedTitle(NSAttributedString(attributedString), for: .normal) | |
registerButton.setAttributedTitle(<#T##title: NSAttributedString?##NSAttributedString?#>, for: .disabled) |
버튼의 그림자를 주는 것보단 이런식으로 disabled일 때 글자 색을 회색? 정도로 해주면 더.. 이쁠 것 같숩니당...ㅎㅅㅎ (해당 부분 디자인을 안만들어놔서 죄송함돠..)
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 final class RegisterViewController: UIViewController { | ||
// MARK: - Property | ||
private let viewModel = RegisterViewModel() |
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: ViewModel
의 실제 객체는 SceneDelegate
에서 만들어서 주입시켜줘야할 것 같아요 !
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.
인정합니다
|
||
let registerButtonBackground = UIView() | ||
registerButtonBackground.backgroundColor = .mhSection | ||
registerButtonBackground.layer.cornerRadius = 12 + 1 |
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.
P3: 12 + 1은 무슨뜻인가요??
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.
registerbutton의 cornerradius가 12이고 registerbuttonbackground의 cornerradius가 12 + 1입니다.
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 final class RegisterViewController: UIViewController { | ||
// MARK: - Property | ||
private var viewModel = RegisterViewModel() |
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.
p3
private let 으로는 못 쓰나용 ??
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.
P2: 생성자에서 viewmodel을 받고있는데 프로퍼티가 뷰모델 객체를 들고있는게 어색하게 느껴집니다..
private var viewModel = RegisterViewModel() | |
private let viewModel: RegisterViewModel |
이게 더 낫지 않을까요??
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
앗 그리고 PR description의 다음버튼 활성화되는 gif 업데이트 해주면 좋을 것 같습니당.ᐟ.ᐟ
|
||
public final class RegisterViewController: UIViewController { | ||
// MARK: - Property | ||
private var viewModel = RegisterViewModel() |
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.
P2: 생성자에서 viewmodel을 받고있는데 프로퍼티가 뷰모델 객체를 들고있는게 어색하게 느껴집니다..
private var viewModel = RegisterViewModel() | |
private let viewModel: RegisterViewModel |
이게 더 낫지 않을까요??
#️⃣ 연관된 이슈
⏰ 작업 시간
Combine, Actor를 학습하는데 시간을 많이 사용해 실제 걸린 시간을 잘 모르겠습니다 ㅠㅠ
실제 구현은 4 - 6 시간 정도 같아요.
📝 작업 내용
📸 스크린샷
⚽️ 트러블 슈팅