-
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
책 생성 화면 작성 #39
책 생성 화면 작성 #39
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.
너무너무 수고 많으셨습니다.ᐟ.ᐟ 확실히 뷰가 복잡하니까.. 코드가 길어지네요... 깔끔하게 잘 작성해주신 것 같아요 🥹 수정하면 좋을 것 같은 지점 조금 리뷰 남겨뒀습니당 !
@@ -179,7 +179,7 @@ | |||
DYLIB_COMPATIBILITY_VERSION = 1; | |||
DYLIB_CURRENT_VERSION = 1; | |||
DYLIB_INSTALL_NAME_BASE = "@rpath"; | |||
ENABLE_MODULE_VERIFIER = YES; | |||
ENABLE_MODULE_VERIFIER = NO; |
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 final class BookCreationViewController: UIViewController { | ||
// MARK: - Property | ||
private let scrollView = UIScrollView() |
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: 해당 scrollview는 사용되지 않는 것 같네용 ~ 사용되지 않는 프로퍼티는 지워주면 좋을 것 같습니당 .ᐟ.ᐟ
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 override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) { | ||
view.endEditing(true) | ||
|
||
super.touchesBegan(touches, with: event) |
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: 여기서 super클래스의 touchesBegan을 불러주신 이유가 무엇인가요?? 따로 호출하지 않아도 되지 않나요..??
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.
애플 문서에서 호출 하라고 되어있어서 그냥 따라쳤는데 ㅋㅋㅋㅋ 혹시 따로 없어도 되나요?
뷰 또는 창에서 새로운 터치가 감지되면 UIKit은 이 메서드를 호출합니다. 많은 UIKit 클래스가 이 메서드를 재정의하여 해당 터치 이벤트를 처리하는 데 사용합니다. 이 메서드의 기본 구현은 메시지를 응답자 체인 위로 전달합니다. 자체 서브클래스를 만들 때는 다음 코드와 같이 직접 처리하지 않는 이벤트를 전달하기 위해 super를 호출하세요. super.touchesBegan(touches, with: event)
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.
아항 저게 터치이벤트를 다음 Responder Chain으로 넘기기위해서 super를 호출하라고 하는 것 같은데, 화면 아무곳을 터치했을 때 키보드만 내려가게 하려먼 저게 없어야될 것 같구,, 만약 다른 곳을 터치했을때 키보드가 내려감과 동시에 터치된 곳에 위치한 버튼이나 이벤트 동작을 실행시키려면 정현님 코드가 맞는 것 같습니다 .ᐟ.ᐟ
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.
엇 완전히 똑같나여..? ㅎㅎ;; 머쓱..
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.
고생하셨습니다 !!
누가 디자인한건지, UI 예쁘네요
@@ -0,0 +1,165 @@ | |||
import UIKit | |||
|
|||
public final class BookCreationViewController: UIViewController { |
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
Public 키워드는 빠져도 될 것 같습니다 !
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.
맞다 Scene에서 불러와서 테스트 하다가 까먹었네요 수정하겠습니다!
return button | ||
}() | ||
private let imageSelectionButton: UIButton = { | ||
let button = UIButton() |
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
이 버튼의 경우 type을 주지 않은 이유가 있으신가요 ?!
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.
아! custom이랑 무슨 차이인지 보려고 했던 코드의 흔적이 남아 혼동을 드렸군요,,, 이것도 수정하겠습니다. 감사합니다 ㅎㅎ
let bookPreviewViewBackground = bookImageView.embededInDefaultBackground( | ||
with: UIEdgeInsets(top: 70, left: 100, bottom: 70, right: 100) | ||
) | ||
view.addSubview(bookPreviewViewBackground) | ||
bookPreviewViewBackground.setAnchor( | ||
top: view.safeAreaLayoutGuide.topAnchor, | ||
leading: view.safeAreaLayoutGuide.leadingAnchor, constantLeading: 16, | ||
trailing: view.safeAreaLayoutGuide.trailingAnchor, constantTrailing: 16, | ||
height: 344 | ||
) | ||
|
||
// 책 제목 | ||
let bookTitleTextFieldBackground = bookTitleTextField | ||
.titledContentView(leftTitle: "책 제목 | ") | ||
.embededInDefaultBackground() | ||
view.addSubview(bookTitleTextFieldBackground) |
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
addSubView는 configureAddSubview()
에서 관리하면 어떨까요 ??
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.
저도 분리하려고 했는데 addSubView랑 constraint를 따로 함수로 구분하면 bookPrevewBackground를 ViewController가 알고 있어야 하더라구요... 근데 ViewController는 background를 뷰 계층에 넣고 나서 아무것도 안해서 이것을 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.
view.addSubview
를 함으로써 계속 레퍼런스 카운팅을 하고 있다면, 저는 오히려 클래스의 프로퍼티에 둬도 괜찮다고 생각하기는 합니다..!
음 다른 분들 의견도 궁금하긴 하네용
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 override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) { |
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.
확실히 lifecycle은 아니긴 합니다 ㅋㅋ 바꿀게욤!
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.
정말 수고 많으셨습니다 !
#️⃣ 연관된 이슈
📝 작업 내용
📒 리뷰 노트
Background생성 로직 분리
처음에는 각각의 뷰에 대한 BackgroundView, BorderView도 따로 들고 있었습니다. 그러다보니 프로퍼티가 너무 많아지더라구요...
근데 생각해보면 background나 borderView는 뷰에 추가하는 데에나 필요하지 주요로직에 대한 관심사와는 거리가 있다고 생각했습니다.
저희 기-술-고-문과의 1분가량의 대화를 통해 분리하는 게 좋겠다고 생각해서 별도의 파일로 분리했습니다.
또한 왼쪽과 오른쪽 타이틀을 추가할 일이 있었고, 이것도 사실 주요 로직과 관련이 없어보여서 별도의 로직으로 뺐습니다.
코드가 많아 보이지만? 대부분 json입니다...
늦어서 죄송합니다...ㅠ
⚽️ 트러블 슈팅
그냥 억까... Font안됨....과 scheme파일 증발? 다시 클론해서 해결했읍니다.... (시간 낭비)
📸 스크린샷