-
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
앱 최초 실행 로직 (Layout X) #43
Conversation
register view controller move to home view controller
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.
정말 수고 많으셨습니당 ~~.ᐟ.ᐟ 읽어보고 리뷰 남겨두었습니다 !
MemorialHouse/MHApplication/MHApplication/Source/App/SceneDelegate.swift
Outdated
Show resolved
Hide resolved
MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift
Show resolved
Hide resolved
MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift
Outdated
Show resolved
Hide resolved
MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift
Outdated
Show resolved
Hide resolved
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.backgroundColor = #colorLiteral(red: 1, green: 1, blue: 1, alpha: 1) | ||
registerButton.layer.borderColor = #colorLiteral(red: 0.7019607843, green: 0.1490196078, blue: 0.1176470588, alpha: 1) | ||
registerButton.layer.borderWidth = 1 | ||
registerButton.layer.cornerRadius = registerButtonFontSize |
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. (혹시 또 구현하고 계실까봐 말씀드립니다...) 배경 넣는 로직 따로 구현해 두었습니다! 나중에 UIView extension으로 embededInDefaultBackground 사용해주시면 될 것같아요!
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.
정현님 테스트 해보았는데, 제꺼 버튼 높이가 24인데 embededInDefaultBackground의 radius가 17로 24/2 보다 커서 테두리가 이질감이 조금 생기네요! 다른 곳에서 배경이 필요하면 적용시켜볼게요 !!
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.
고생하셨습니다 윤철님 !!
제 의견 및 다른 분들 코멘트에 추가 의견 남겨두었습니다 ~!
MemorialHouse/MHApplication/MHApplication/Source/App/SceneDelegate.swift
Outdated
Show resolved
Hide resolved
MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift
Outdated
Show resolved
Hide resolved
MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift
Outdated
Show resolved
Hide resolved
MemorialHouse/MHPresentation/MHPresentation/Source/Register/RegisterViewController.swift
Outdated
Show resolved
Hide resolved
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.
고생하셨습니다 !!
@@ -12,7 +13,13 @@ final class SceneDelegate: UIResponder, UIWindowSceneDelegate { | |||
guard let windowScene = (scene as? UIWindowScene) else { return } | |||
|
|||
window = UIWindow(windowScene: windowScene) | |||
window?.rootViewController = 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.
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.
늦은 밤 고생하셨읍니다!! tag관련해서 댓글 달았읍니다!
switch textField.tag { | ||
case UITextField.Tag.register: | ||
if string.isEmpty { | ||
registerButton.isEnabled = false | ||
} else { | ||
registerButton.isEnabled = true | ||
} | ||
default: | ||
break | ||
} | ||
|
||
return true | ||
} | ||
} | ||
|
||
// MARK: - Tag for UITextField | ||
|
||
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.
P2. tag가 꼭 필요한가요? == 연산자로 대체할 수 있을 것같아서요!, 그리고 textfield가 하나만 쓰인다면 바로 empty인지만 확인해도 되지 않을까하는 생각입니다,,,
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.
혹시 TextField가 추가될지 몰라서 저렇게 해두었는데, 정현님 말씀도 맞는 것 같아요.
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 initialViewController = UserDefaults.standard.object(forKey: Constant.houseName) == nil | ||
? RegisterViewController() | ||
: 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.
P3: 삼항연산자가 이렇게 내려쓰여진게 어색한 것 같아요.. 처음보는 스타일인 것 같네용.. 혹시 if 조건문을 사용하지 않으신 이유가 있으신걸까욥..??
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.
저 부분 제가 제의를 드렸었는데,
삼항 연산자는 swift에서 저렇게 쓰더라구요!
다른 분들 보시기에 가독성 별로인 거 같으면 기존 걸로 되돌려도 되긴 합니다
저는 if else보다 저게 가독성이 더 좋은데,
@iceHood 정현님은 어떠신가요?
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 initialViewController = if UserDefaults.standard.object(forKey: Constant.houseName) == nil {
RegisterViewController()
} else {
HomeViewController()
}
|
||
public final class RegisterViewController: UIViewController { | ||
// MARK: - Properties | ||
|
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: 각자의 코드 스타일인 것 같긴 하지만,, mark 주석 아래 개행은 없애는게 다른 분들과 통일감을 주는 코드가 될 수 있을 것 같아요 .ᐟ.ᐟ (아래 mark 주석들도 포함입니당 ㅎㅅㅎ)
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 enum Constant { | ||
public static let houseName = "houseName" | ||
} |
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.
Constant에서 폴더링을 해서 UserDefaults로 키 값 상수를 관리하면 어떨까요 ?
public enum Constant { | |
public static let houseName = "houseName" | |
} | |
public enum Constant { | |
public enum UserDefaultsKey { | |
public static let houseName = "houseName" | |
} | |
} |
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.
고생많으셨습니다... 수정?이나 다됐다 싶으시면 머지 하시면 될듯싶습니다...!
#️⃣ 연관된 이슈
📝 작업 내용
📸 스크린샷
저장소 입력 없이 <다음> 버튼 누르는 경우 alert
저장소 입력 후 HomeViewController로 이동
UserDefaults가 존재하는 상황에서는 바로 HomeViewController로 이동