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

Feature/#6 MVVM Refectoring #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Feature/#6 MVVM Refectoring #7

wants to merge 5 commits into from

Conversation

SeorinY
Copy link
Collaborator

@SeorinY SeorinY commented Nov 26, 2022

관련이슈 🍎🍏

  • 세미나 1차 과제 MVVM ( RX Study 과제 )
  • Sign in
  • Sign Up
  • Login Confirm

흠..

  • Sign in -> Login Confirm 으로 넘어가는 기능은 무사히 구현했습니다
  • Sign Up -> Login Confirm 으로 넘어갈 때 Confirm> 화면으로 넘어가는 방식이랑 똑같이 했는데 잘 안 되네요...! 혹시 이유를 알 수 있을까요🥹

Copy link

@pastapeter pastapeter left a comment

Choose a reason for hiding this comment

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

ㅎㅇㅌㅎㅇㅌ
뭐가 잘안되는디?

struct SignInViewModel{
var name : Observable<String>
let disposeBag = DisposeBag()

Choose a reason for hiding this comment

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

struct로 한 이유가 있을까용?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

딱히 이유는 없어용!
굳이 찾자면... Struct는 스택메모리에 저장돼서 좀 가벼운 느낌이라..?
Struct 로 가능한 것 들은 최대한 Struct 로 하려고 하는데
찾아보니까 보통 ViewModel 구현할 때 Class 로 하는 것 같긴해요...ㅎ
이유는 좀 더 공부해보겠습니당

Choose a reason for hiding this comment

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

wwdc16 understanding swift performance 나
struct 내 reference type이 많을 경우 어떤 오버헤드가 발생하는지 검색해보면 이유를 알수있을수도요 ㅎ


let _ = Observable
.combineLatest(fetchEmail, fetchPassword)
.subscribe(onNext: {

Choose a reason for hiding this comment

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

map -> bind 로 바꿀수 있을것같아요

Choose a reason for hiding this comment

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

코드가 읽기 편하려면 최대한 일편적인 indenentation을 하려고 노력하면 좋습니당.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

핫 indenentation 피드백 너무 고맙슴당🥹

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

map -> bind 피드백 주셨는데 혹시 위 코드에서 map이 combineLatest 가리키는 건가용?

Observable
.combineLatest(fetchPassword, fetchChechPassword)
.map{ $0 == $1}
.subscribe { checkCondition.onNext($0) }

Choose a reason for hiding this comment

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

이거를 바로 isSamePW로 바인드해주는것 어떤가용

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
Member

@cchanmi cchanmi left a comment

Choose a reason for hiding this comment

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

고생하셧습니다... 알엑스 어려웠을 텐데 서린 최고.
목요일에 민재한테도 코드리뷰 받자 ㅎㅎ

Comment on lines 22 to +25
override func viewDidLoad() {
super.viewDidLoad()
view.backgroundColor = .white

setLoginCheckViewControllerLayout()
confirmBtn.addTarget(self, action: #selector(didTapConfirmButton), for: .touchUpInside)
}

public func configEmail(_ email : String){
welcomeLabel.text = "\(email)님\n 환영합니다"
setUI()
setLayout()
Copy link
Member

Choose a reason for hiding this comment

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

viewDidLoad()에 setDataBindRx 호출 안해줘서 안되는거아냐?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이거는 다른 뷰턴에서 호출해주는 함수여서! 좀이따가 조금 더 구체적으로 톡방에 올릴게!

Comment on lines +64 to +67
@objc private func didTapConfirmButton() {
self.dismiss(animated: true)
delegate?.dismissNavigationController()
}
Copy link
Member

@cchanmi cchanmi Nov 30, 2022

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.

나 류트뷰컨 바꿔주는 방식으로 안 하고
모달 -> 네비게이션컨트롤러 -> Auth 뷰컨들
이렇게 했었어서! 담에는 루트뷰컨 바꾸는 방식으로 해볼게!

}else{
return 50
}
indexPath.row == 0 ? 73 : 50
Copy link
Member

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.

코드 리뷰 받아서 고친~! ^~^

@lsj8706 lsj8706 removed their request for review August 23, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants