Skip to content
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

Merged
merged 16 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
import UIKit
import MHFoundation
import Combine

final class MHRegisterView: UIView {
static let textfieldSubject = PassthroughSubject<String, Never>()
static let buttonSubject = PassthroughSubject<Bool, Never>()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3. 제가 Combine을 잘 몰라서 그러는데 원래 static으로 구독을 관리하나요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1
엇 저도 Static 사용하지 않는 걸로 알고 있습니다 !
static을 사용하신 이유가 어떻게 되나요 ?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 수정했습니다 ~~ ㅎㅎ


// MARK: - Property
private let coverImageView: UIImageView = {
let backgroundImageView = UIImageView()
backgroundImageView.image = UIImage.pinkBook

return backgroundImageView
}()
private static let registerButtonFontSize: CGFloat = 12
private static let registerTextFieldFontSize: CGFloat = 24
private let registerTextLabel: UILabel = {
let registerFont = UIFont.ownglyphBerry(size: registerTextFieldFontSize)

let textLabel = UILabel()
textLabel.text = """
추억을 간직할
기록소 이름을 작성해주세요
"""
textLabel.textAlignment = .center
textLabel.font = registerFont
textLabel.numberOfLines = 2

return textLabel
}()
let registerTextField: UITextField = {
let registerFont = UIFont.ownglyphBerry(size: registerButtonFontSize)

let textField = UITextField()
textField.font = registerFont
textField.borderStyle = .none
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3. border가 없으면 설정 안하는 것과는 차이가 없을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default가 none이네요? 삭제했습니다 ~


var attributedText = AttributedString(stringLiteral: "기록소")
attributedText.font = registerFont
textField.attributedPlaceholder = NSAttributedString(attributedText)

textField.tag = UITextField.Tag.register

return textField
}()
let registerButton: UIButton = {
let registerButton = UIButton(type: .custom)

var attributedString = AttributedString(stringLiteral: "다음")
attributedString.font = UIFont.ownglyphBerry(size: registerButtonFontSize)
attributedString.strokeColor = UIColor.mhTitle

registerButton.setAttributedTitle(NSAttributedString(attributedString), for: .normal)

registerButton.backgroundColor = .mhSection
registerButton.layer.borderColor = UIColor.mhBorder.cgColor
registerButton.layer.borderWidth = 1
registerButton.layer.cornerRadius = registerButtonFontSize

return registerButton
}()

// MARK: - Initializers
init() { super.init(frame: .zero) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 해당 initializer는 없어도 되지 않나욥?? 어떨때 사용되는 이니셜라이저인가요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하려고 만들었다가 사용하지 않는 코드가 됐는데 제거를 못했네요
제거 했습니다 ~


override init(frame: CGRect) {
super.init(frame: frame)

setup()
configureAddSubviewAndConstraints()
}

required init?(coder: NSCoder) {
fatalError("error: mhregisterview requires init(coder:)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 저희 컨벤션에 따라서 required init 함수 내부도 위 오버라이딩 한 이니셜 라이저와 똑같게 구현해줘야할 것 같습니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다! 수정했습니다 ~!

}

// MARK: - Setup
private func setup() {
addTouchEventToRegisterButton(registerButton)
addEditingChangedEventToRegisterTextField(registerTextField)
coverImageView.isUserInteractionEnabled = true
}

// MARK: - Configure
private func configureAddSubviewAndConstraints() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: 해당 함수 내부에 Constraints를 잡아주거나 addSubView를 해주는 동작 말고도 layer를 설정하는 등의 부가적인 동작들이 존재하는 것 같습니다. 함수를 분리해보는게 좋아보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

분리했습니다요 ~~

coverImageView.setWidthAndHeight(width: self.frame.width, height: self.frame.height)
self.addSubview(coverImageView)

coverImageView.addSubview(registerTextLabel)
registerTextLabel.setAnchor(
leading: self.leadingAnchor,
constantLeading: 80,
trailing: self.trailingAnchor,
constantTrailing: 40,

height: MHRegisterView.registerTextFieldFontSize * 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2. 이렇게 고정된 폰트 사이즈로 높이를 잡는 것이 일반적인지 살짝 의문이 드네용... 만약 사용자가 글씨 보기를 키우면 어떻게 보이나요? IntrinsicSize 대신 폰트사이즈 계산으로 정하신 이유가 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

폰트 크기를 수정할 때 버튼 크기도 변경하주고싶어서 이렇게 했는데, 수정했습니다 ~~

)

let registerTextFieldBackground = registerTextField
.embededInDefaultBackground(with:
UIEdgeInsets(top: 0, left: 5, bottom: 0, right: 5))
coverImageView.addSubview(registerTextFieldBackground)
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
)
Copy link
Collaborator

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를 같은 줄에 쓰는 게 개인적으로 가독성이 더 좋다고 보긴 합니다 !!

(위아래 다른 레이아웃 잡는 부분도 마찬가지입니다 !!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 같은줄에 쓰는게 좋은 거 같아서 전체적으로 수정했습니다!


coverImageView.addSubview(registerButtonBackground)
registerButtonBackground.setAnchor(
top: registerTextFieldBackground.bottomAnchor,
constantTop: 10,
leading: self.leadingAnchor,
constantLeading: MHRegisterView.registerTextFieldFontSize * 11,

width: MHRegisterView.registerButtonFontSize * 4 + 8,
height: MHRegisterView.registerButtonFontSize * 2 + 6
)
}

private func addTouchEventToRegisterButton(_ button: UIButton) {
let uiAction = UIAction { [weak self] _ in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: 해당 함수 내부에서만 사용되는 지역변수이긴 하지만 정확히 어떤 동작을 하는지 변수명만 봤을때 알 수 없을 것 같아요.. 네이밍을 다시 지어보면 좋을 것 같습니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegisterButton에 터치 이벤트를 준다는 의미인데 혹시 추천해주실만한게 있을까요?

guard let houseName = self?.registerTextField.text, !houseName.isEmpty else { return }

if self?.checkNameValidity(houseName) ?? false {
Copy link
Collaborator

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문으로 해줘도 되지 않을까..? 라는 생각입니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 조금 더 공부해볼게요 !!

Copy link
Collaborator

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 { 코드들... 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 수정했습니다 ~~

MHRegisterView.buttonSubject.send(true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: View끼리 바인딩을하고 view에서 특정 값을 능동적으로 방출하는 방식이 어색해보입니다.. View끼리 데이터를 주고받을 떄에는 메서드를 통해서도 충분히 가능하지 않을까요??

Copy link
Collaborator Author

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: UserDefault에 값을 저장하는 로직은 ViewModel에 들어가는게 더 적합해보입니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 인정합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호! Register 로직에서 ViewModel을 생성하지 않아서 Viewcontroller로 로직을 옮겼는데 어떨까요?

} else {
MHRegisterView.buttonSubject.send(false)
}
}
registerButton.addAction(uiAction, for: .touchUpInside)
}

private func addEditingChangedEventToRegisterTextField(_ textfield: UITextField) {
let uiAction = UIAction { _ in
guard let inputText = textfield.text else { return }
MHRegisterView.textfieldSubject.send(inputText)

switch textfield.tag {
case UITextField.Tag.register:
if inputText.isEmpty == true {
self.registerButton.isEnabled = false
} else {
self.registerButton.isEnabled = true
}
Copy link
Collaborator

@iceHood iceHood Nov 16, 2024

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이 눌릴 것같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정했습니다 ~~

default:
break
}
}
registerTextField.addAction(uiAction, for: .editingChanged)
}

private func checkNameValidity(_ name: String) -> Bool {
return name.count < 11 ? true : false
}
}

// MARK: - Tag for UITextField
extension UITextField {
enum Tag {
static let register = 0
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: 해당 화면에선 UITextField가 하나밖에 존재하지 않는데 Tag를 사용해야하는 이유가 무엇인지 궁금합니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제 완!

Original file line number Diff line number Diff line change
@@ -1,116 +1,52 @@
import UIKit
import MHFoundation
import Combine

public final class RegisterViewController: UIViewController {
// MARK: - Properties
private static let registerButtonFontSize: CGFloat = 12
private static let registerTextFieldFontSize: CGFloat = 24
private let registerTextField: UITextField = {
let registerFont = UIFont.ownglyphBerry(size: registerButtonFontSize)

let textField = UITextField()
textField.font = registerFont
textField.borderStyle = .roundedRect

var attributedText = AttributedString(stringLiteral: "기록소")
attributedText.font = registerFont
textField.attributedPlaceholder = NSAttributedString(attributedText)

textField.tag = UITextField.Tag.register

return textField
}()
private let registerButton: UIButton = {
let registerButton = UIButton(type: .custom)

var attributedString = AttributedString(stringLiteral: "다음")
attributedString.font = UIFont.ownglyphBerry(size: registerButtonFontSize)
attributedString.strokeColor = UIColor.mhTitle

registerButton.setAttributedTitle(NSAttributedString(attributedString), for: .normal)

registerButton.backgroundColor = UIColor.mhSection
registerButton.layer.borderColor = UIColor.mhBorder.cgColor
registerButton.layer.borderWidth = 1
registerButton.layer.cornerRadius = registerButtonFontSize

return registerButton
}()
var subscriptions = Set<AnyCancellable>()

// MARK: - Property
var registerView = MHRegisterView()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2
private let으로 가져도 좋을 거 같습니다

Suggested change
var registerView = MHRegisterView()
private let registerView = MHRegisterView()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View, Controller 코드를 합쳤습니다!

var username = ""

// MARK: - Lifecycle
public override func viewDidLoad() {
super.viewDidLoad()

setup()
configureAddSubView()
configureAddSubview()
configureConstraints()
}

// MARK: - Setup
private func setup() {
view.backgroundColor = .baseBackground

addTouchEventToRegisterButton(registerButton)
}

// MARK: - Configure
private func configureAddSubView() {
view.addSubview(registerTextField)
view.addSubview(registerButton)
}
private func configureConstraints() {
registerTextField.setHeight(RegisterViewController.registerTextFieldFontSize)
registerTextField.setWidth(RegisterViewController.registerTextFieldFontSize * 8)
registerTextField.setCenter(view: view)

registerButton.setHeight(RegisterViewController.registerButtonFontSize * 2)
registerButton.setWidth(RegisterViewController.registerButtonFontSize * 4)
registerButton.setTop(anchor: registerTextField.bottomAnchor, constant: 6)
registerButton.setLeading(anchor: registerTextField.trailingAnchor, constant: -6)
}

private func addTouchEventToRegisterButton(_ button: UIButton) {
let uiAction = UIAction { [weak self] _ in
// TODO: - 저장소 중복 체크

guard let houseName = self?.registerTextField.text, !houseName.isEmpty else { return }

let userDefaults = UserDefaults.standard

userDefaults.set(houseName, forKey: Constant.houseNameUserDefaultKey)
let homeViewModel = HomeViewModel(houseName: houseName)
self?.navigationController?.pushViewController(HomeViewController(viewModel: homeViewModel), animated: false)
self?.navigationController?.viewControllers.removeFirst() // inactive back to register view
}
MHRegisterView.textfieldSubject.sink { text in
self.username = text
}.store(in: &subscriptions)

button.addAction(uiAction, for: .touchUpInside)
}
}

// MARK: - UITextFieldDelegate
extension RegisterViewController: UITextFieldDelegate {
public func textField(
_ textField: UITextField,
shouldChangeCharactersIn range: NSRange,
replacementString string: String) -> Bool {
switch textField.tag {
case UITextField.Tag.register:
if string.isEmpty {
registerButton.isEnabled = false
MHRegisterView.buttonSubject.sink { isValid in
if isValid {
let homeViewController = HomeViewController(viewModel: HomeViewModel(houseName: self.username))
self.navigationController?.pushViewController(homeViewController, animated: false)
Copy link
Collaborator

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을 띄워주면 어떨 것 같나용 ??

아니면 이 방식이 자연스러운 걸까요 ?

Copy link
Collaborator Author

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에서 제거해주었습니다! 혹시 이러한 방식의 문제점이 있으면 말씀해주세요!

self.navigationController?.viewControllers.removeFirst() // inactive back to register view
} else {
registerButton.isEnabled = true
// TODO: - 기록소 이름 조건이 안맞는 부분 처리

Check warning on line 34 in MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift

View workflow job for this annotation

GitHub Actions / SwiftLint

Todo Violation: TODOs should be resolved (- 기록소 이름 조건이 안맞는 부분 처리) (todo)

Check warning on line 34 in MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift

View workflow job for this annotation

GitHub Actions / SwiftLint

Todo Violation: TODOs should be resolved (- 기록소 이름 조건이 안맞는 부분 처리) (todo)
}
default:
break
}
}.store(in: &subscriptions)

return true
registerView = MHRegisterView(
frame: CGRect.init(x: 0, y: 0, width: view.bounds.width - 50, height: view.bounds.height - 640))
Copy link
Collaborator

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으로 선언하는게 더 가독성이 좋을 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view와 viewcontroller를 합치는 방향으로 수정했습니다 ~~

}
}

// MARK: - Tag for UITextField
extension UITextField {
enum Tag {
static let register = 0

private func configureAddSubview() {
self.view.addSubview(registerView)
}

private func configureConstraints() {
registerView.setTop(anchor: self.view.topAnchor, constant: 320)
registerView.setBottom(anchor: self.view.bottomAnchor, constant: 320)
registerView.setLeading(anchor: self.view.leadingAnchor, constant: 25)
registerView.setTrailing(anchor: self.view.trailingAnchor, constant: 25)
}
}
Loading