-
Notifications
You must be signed in to change notification settings - Fork 2
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] 화이트보드에 그림 추가 기능 구현(2차) #87
Conversation
ec4526c
to
d4a5a0d
Compare
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.
어려운 로직인데 잘 구현해 주신 거 같습니다!
고생하셨습니다!
|
||
final class DrawingView: UIView { | ||
private var drawingLayer = CAShapeLayer() | ||
private var drawingPath = UIBezierPath() |
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
로 선언된 이유가 궁금합니다.
추후에 재할당될 가능성이 있는 걸까요?
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으로 바꾸겠습니다
drawingView | ||
.addToSuperview(scrollView) | ||
.edges(equalTo: canvasView) | ||
|
||
toolbar | ||
.addToSuperview(view) | ||
.horizontalEdges(equalTo: view, inset: 22) |
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.
조이가 만들어놓은 horizontalMargin을 활용하면 좋을 거 같습니다!
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.
바로 수정하겠습니다!!
drawingView | ||
.addToSuperview(scrollView) | ||
.edges(equalTo: canvasView) | ||
|
||
toolbar | ||
.addToSuperview(view) | ||
.horizontalEdges(equalTo: view, inset: 22) | ||
.bottom(equalTo: view.safeAreaLayoutGuide.bottomAnchor, inset: 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.
여긴도 .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.
좋은 생각입니다 :)
drawingView | ||
.addToSuperview(scrollView) | ||
.edges(equalTo: canvasView) | ||
|
||
toolbar | ||
.addToSuperview(view) | ||
.horizontalEdges(equalTo: view, inset: 22) | ||
.bottom(equalTo: view.safeAreaLayoutGuide.bottomAnchor, inset: 0) | ||
.height(equalTo: 40) |
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.
컨벤션 정할 때 높이 등의 제약조건을 enum으로 관리하기로 한거같습니다!
위에 canvasView
의 크기도 정의하면 좋을거같아용
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쪽 컨벤션에서 놓친게 많군요..! 수정하겠습니다
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.
주말까지 완전 열심히 하신 띵똥 !!!! 본받겠습니다. .....
엄청 복잡한 로직을 담당해주신 것 같아요 .......... 👍🏻 정말 수고하셨습니다 !!! 구웃 ~
/// 그리기를 시작하는 메서드 | ||
/// 선의 굵기를 설정합니다. | ||
/// - Parameter width: 선의 너비 | ||
func setLineWidth(width: CGFloat) |
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.
저희 코드 컨벤션 정할 때 함수명에 get, set 지양하기로 하지 않았었나용 ? 🥺
만약 lineWidth = 5
처럼 선 굵기에 대한 초기값이 정해져있는데 업데이트를 위해 set 함수가 필요한 것이라면 update 혹은 configure, apply 라는 메서드명은 어떠신지요 ??
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.
앗 그렇네요..!
말씀해주신 것처럼 초기값이 정해져 있으니 update라는 네이밍이 괜찮아보입니다. 수정하겠습니다!
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.
func lineWidth(to width: Int)
func configureLineWidth(to width: Int)
조이의 코멘트를 보고 추천 남깁니다~
@@ -20,14 +20,17 @@ public protocol ManageWhiteboardObjectUseCaseInterface { | |||
/// 화이트보드 객체를 추가하는 메서드 | |||
/// - Parameter whiteboardObject: 추가할 화이트보드 객체 | |||
/// - Returns: 추가 성공 여부 | |||
@discardableResult |
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.
해당 코멘트 영향으로 @discardableResult를 붙인건가용 ??
혹시 테스트 코드 외에 실제 UseCase 함수를 사용할 때에도 결과 값을 사용하지 않는 부분이 있을지 궁금합니다. !
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.
우선은 실패 처리를 위해 Bool 타입을 반환하도록 설계했습니다. 다만 아직 실패했을 때에 대한 정책이 정해진 것이 없어 반환값을 사용하는 곳이 없습니다. (테스트 코드 제외) 따라서 일단 discardableResult를 붙여놓았습니다!
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.
주말까지 고생 많으셨습니다!!
덕분에 ViewController와 ViewModel부분을 편하게 이용할 수 있을 것 같습니다ㅜㅜ 최고!!
let previousTool = currentToolSubject.value | ||
if previousTool == tool { | ||
currentToolSubject.send(nil) | ||
} else { | ||
currentToolSubject.send(tool) | ||
} |
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.
send(nil)을 통해 nil값에 따라 처리가 필요한 거 겠죠!?
+
삼항 연산자는 어떻게 생각하시나요??
let previousTool = currentToolSubject.value | |
if previousTool == tool { | |
currentToolSubject.send(nil) | |
} else { | |
currentToolSubject.send(tool) | |
} | |
let selectTool = currentToolSubject.value == tool ? nil: tool | |
currentToolSubject.send(selectTool) |
이런 느낌으로!!
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.
깔끔해서 좋은 것 같습니다! 동~
@objc private func handleDrawingGesture(sender: UIPanGestureRecognizer) { | ||
let point = sender.location(in: self) | ||
|
||
switch sender.state { | ||
case .began: | ||
previousPoint = point | ||
delegate?.drawingViewDidStartDrawing(self, at: point) | ||
drawLine(to: point) | ||
case .changed: | ||
delegate?.drawingView(self, at: point) | ||
drawLine(to: point) | ||
case .ended: | ||
delegate?.drawingViewDidEndDrawing(self) | ||
previousPoint = nil | ||
default: | ||
break | ||
} | ||
} |
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.
스프린트 때 PanGesture를 사용해 봤었는데, 이런 드래그 이벤트같은 것들은 UIKit에서는 거의 PanGesture로 처리가 가능하군요??
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.
넵 그렇습니다!!!
/// 그리기를 시작하는 메서드 | ||
/// 선의 굵기를 설정합니다. | ||
/// - Parameter width: 선의 너비 | ||
func setLineWidth(width: CGFloat) |
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.
func lineWidth(to width: Int)
func configureLineWidth(to width: Int)
조이의 코멘트를 보고 추천 남깁니다~
public init() { | ||
points = [] | ||
lineWidth = 5 | ||
} | ||
|
||
public func setLineWidth(width: CGFloat) { | ||
lineWidth = width | ||
} |
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.
앗 그렇네요..??ㅋㅋㅋ
// TODO: - 딴과 논의 필요 | ||
let objectId: UUID |
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.
object ID만 갖고 있을지 object자체를 갖고 있을지 논의가 필요한 거라고 예상되네요!
만약 변경 데이터가 왔을 때 비교 후 같을 경우에는 굳이 변경 시켜줄 필요 없다고 생각해서 저는 object자체를 갖고 있게 했는데요!! ID만 갖고있다면 변경 데이터가 왔을 때 무조건 뷰를 변경 시켜줄 것이라고 예상 했습니다!!
근데, 같은 값으로 변경하는 코드가 뷰를 변경시키지 않고 넘어간다면 ID만 갖고있는 것도 좋을 것 같아요!
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가 직접 Object를 관리하지 않으면 좋겠다고 생각했습니다!
변경사항이 생길때마다 현재 설계에서는 ManageWhiteboardObjectUseCase에서 publish를 통해 viewModel에게 알려주고 있습니다! 이를 다시 ViewModel -> VC에게 알려주면, VC가 해당 오브젝트 ID를 가지고 있는 View를 찾아서 frame을 바꿔주면 되지 않을까 합니다. 생각해보니, VC에서 [오브젝트 ID: WhiteboardObjectView] 형태로 관리한다면, ObjectID도 가지고 있지 않아도 괜찮을 것 같습니다!
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.
좋습니다!!
VC이 [오브젝트 ID: WhiteboardObjectView] 형태로 관리한다는 것이 보통 VC의 역할이겠죠??
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.
더 좋은 방법이 있을 수도 있으니까 오늘 내일 같이 논의해봐도 좋을 것 같습니다!!
viewModel.output.updatedWhiteboardObjectPublisher | ||
.receive(on: DispatchQueue.main) | ||
.sink { _ in | ||
// TODO: - 오브젝트 수정 시 동작 구현 | ||
} | ||
.store(in: &cancellables) | ||
|
||
viewModel.output.removedWhiteboardObjectPublisher | ||
.receive(on: DispatchQueue.main) | ||
.sink { _ in | ||
// TODO: - 오브젝트 삭제 시 동작 구현 | ||
} | ||
.store(in: &cancellables) |
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.
TODO니까 eemddeks이라던가 아니면 eemddeks 또는 eemddeks가 해줄지도요.!.!.!
viewModel.output.addedWhiteboardObjectPublisher | ||
.receive(on: DispatchQueue.main) | ||
.sink { [weak self] object in | ||
guard let objectView = self?.objectViewFactory.create(with: object) else { return } | ||
self?.addObjectView(objectView: objectView) | ||
} | ||
.store(in: &cancellables) |
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.
canvasView가 추가되어, ScrollView안에서 스크롤이 가능할 것 같은데요,
오브젝트를 뷰가 아니라 canvasView에 추가하지 않아도 될까요??
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 addObjectView(objectView: WhiteboardObjectView) {
canvasView.addSubview(objectView)
}
요렇게 addObjectView
를 통해 canvasView에 추가하고 있습니다!
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.
헉 확인했습니다!!
구우욷
94f6f21
to
e8e129b
Compare
🌁 Background
📱 Screenshot
SE3.mov
13mini.mov
16Pro.mov
👩💻 Contents
✅ Testing
📝 Review Note
toolBar의 작동 방식 수정 (커밋 링크: bee7bef)
그림 오브젝트 생성 전 실시간 그림 그리기 (커밋 링크: 13873ec, d9509f5)
UIGraphicsImageRenderer로 이미지를 렌더링 하면 네 모서리쪽 선들이 잘리는 현상 (커밋 링크: 6457079)
📣 Related Issue
📬 Reference