Feat: memo main view 구현 #17
Conversation
- 통일성을 위해 변경하였습니다.
- 기존 MemoCell에서 특정 렌더링 문제가 발생했으나, 파일 삭제 후 동일한 코드로 다시 생성하자 정상 동작함. 원인 불명이나 현재 정상 작동 중.
- MemoCell이 사용하는 모든 상태와 액션이 intent에 의존하고 있어, 클로저 대신 intent 전체를 주입하도록 변경 - MemoListView에서 전달하던 클로저 제거로 코드 간결화 - MemoCell 내부에서 intent.state를 통해 isEditing, isMemoHidden 계산
- 텍스트 필드 포커싱을 Store 상태에서 View 사이드 이펙트로 - MemoStore에서 사이드 이펙트 발생 시 Combine을 통해 View로 전달하도록 변경 - MemoView에서 .onReceive를 통해 포커스 UI 처리
- `Router`를 `MemoStore`에서 제거하고, `MemoView`에서 `@Environment`로 관리 - 네비게이션 관련 사이드 이펙트를 View에서 처리하도록 수정
| // TODO: MemoRepository를 어디서 주입할 지 고민하기 | ||
| let memoRepository: MemoRepositoryProtocol = MemoRepository() |
There was a problem hiding this comment.
해당 인스턴스를 어디에서 주입하는 것이 적절할지 고민 중입니다.
현재는 여러 화면에서 공통적으로 사용될 수 있다고 판단하여, 우선 ViewFactory에서 생성 및 주입하도록 구성했습니다.
더 나은 위치가 있다면 피드백 부탁드립니다!
There was a problem hiding this comment.
기존에는 생성해야하는 것들이 화면/Intent 뿐이어서 View를 만든다는 의미의 ViewFactory로 명명했는데요! Repository 및 혹시 추후에 생길 수도 있는 다양한 의존성들의 주입을 이곳에서 진행하고, DIContainer 로 이름을 변경하는 것은 어떨지 제안해봅니다!
There was a problem hiding this comment.
오호랏 전 이 ViewFactory가 팩토리 메소드 패턴을 채택한 아이라고 생각해서 repository와 같은 인스턴스들을 주입해도 된다고 생각했습니다! +) 네이밍은 크으게 상관 없어보이는데 바꿔도 의미 상 틀린 건 없어서 좋습니당
There was a problem hiding this comment.
PR의 규모는 작게 나누는 것이 좋다는 것을 알고 있지만, 처음 접하는 아키텍처로 구현을 진행하다 보니 어디서 끊어야 할지 감을 잡지 못해 PR이 다소 커지게 되었습니다.🥲
양해 부탁드립니다 🙏
처음에는 “메모와 관련된 뷰를 구현하자”는 범위로 시작했는데, 진행하면서 UI 흐름, 상태 관리, 사이드 이펙트 처리 등 뷰에 밀접하게 연관된 로직들이 연쇄적으로 추가되다 보니 범위가 넓어졌습니다.
혹시 다른 분들은 뷰 작업 시 어떤 기준으로 작업 범위를 나누는지 궁금합니다.
| final class MockMemoRepository: MemoRepositoryProtocol { | ||
| private var mockMemos: [Memo] = Memo.sampleMemos | ||
|
|
||
| func save(memo: Memo) -> Result<Void, Error> { | ||
| mockMemos.append(memo) | ||
| return .success(()) | ||
| } | ||
|
|
||
| func fetchAllMemos() -> Result<[Memo], Error> { | ||
| return .success(mockMemos) | ||
| } | ||
|
|
||
| func update(memo: Memo) -> Result<Void, Error> { | ||
| if let index = mockMemos.firstIndex(where: { $0.id == memo.id }) { | ||
| mockMemos[index] = memo | ||
| return .success(()) | ||
| } | ||
| return .failure(NSError(domain: "MockMemoRepository", code: 0, userInfo: [NSLocalizedDescriptionKey: "Memo not found."])) | ||
| } | ||
|
|
||
| func delete(memo: Memo) -> Result<Void, Error> { | ||
| mockMemos.removeAll { $0.id == memo.id } | ||
| return .success(()) | ||
| } | ||
|
|
||
| func deleteAll() { | ||
| mockMemos.removeAll() | ||
| } | ||
| } |
There was a problem hiding this comment.
뷰 테스트를 위해 Mock 데이터를 제공하는 레포지토리를 작성해두었습니다.
이후 실제 사용 시에는 레포지토리 주입 부분만 실제 구현체로 교체해주면 됩니다.
| private var shouldShowMemo: Bool { | ||
| // 회고가 없으면 무조건 메모를 보여주고, | ||
| // 회고가 있는 경우엔 사용자가 펼쳤을 때만 보여준다 | ||
| !isMemoHidden || !hasRetrospection | ||
| } |
There was a problem hiding this comment.
아래에서 사용되는 분기 조건을 더 명확하게 나누고 싶었지만, 로직 구조에 대한 추가적인 고민이 필요할 것 같아 우선은 shouldShowMemo라는 상태를 나타내는 변수로 추출하여 가독성을 높였습니다.
| // MARK: Intent | ||
| enum Intent { | ||
| case showRetrospectionView // 회고 화면으로 | ||
| case showComfieZoneSettingView // 컴피존 설정 화면으로 | ||
| case deletePopup(PopupIntent) | ||
| case memoInput(MemoInputIntent) | ||
| case memoCell(MemoCellIntent) | ||
|
|
||
| case onAppear | ||
| case backgroundTapped | ||
| case comfieZoneSettingButtonTapped | ||
|
|
||
| enum PopupIntent { | ||
| case confirmDeleteButtonTapped | ||
| case cancelDeleteButtonTapped | ||
| } | ||
|
|
||
| enum MemoInputIntent { | ||
| case updateNewMemo(String) | ||
| case memoInputButtonTapped | ||
| } | ||
|
|
||
| enum MemoCellIntent { | ||
| case editButtonTapped(Memo) | ||
| case retrospectionButtonTapped(Memo) | ||
| case deleteButtonTapped(Memo) | ||
| case editingCancelButtonTapped | ||
| } | ||
| } |
There was a problem hiding this comment.
Memo 관련 뷰의 intent를 하나의 Intent enum으로 관리하려다 보니 케이스 수가 많아져 가독성이 떨어지는 문제가 있었습니다.
Store를 분리하는 방안도 고려했지만, 우선은 intent 내부에서 각 상황별로 한 번 더 나누어 enum으로 세분화해 구조를 정리했습니다.
아래 action 분기 또한 같은 이유로 내부에서 한 번 더 분리해두었습니다.
이후 규모가 더 커질 경우에는 MemoCellStore처럼 별도의 Store로 분리하는 것도 좋은 방향이라고 생각합니다.
There was a problem hiding this comment.
네! 확실히 Intent의 내용이 많아지니 한눈에 읽히기 어려운 것 같네요! Cell이 맡고 있는 의도가 많다면 분리하는 것도 좋아보입니다.
There was a problem hiding this comment.
해당 작업은 이후 작업하면서 분리해보겠습니다!
| // 화면 전환을 위한 router | ||
| private let router: Router |
There was a problem hiding this comment.
화면 전환과 같은 UI 관련 동작은 View의 책임이라고 판단하여, Store에서는 이벤트 트리거만 처리하고, 실제 화면 전환은 View에서 직접 Router를 환경 변수로 받아 실행하도록 구성했습니다.
| private func performSideEffect(for action: SideEffect) { | ||
| sideEffectPublisher.send(action) | ||
| } | ||
| } |
There was a problem hiding this comment.
performSideEffect(for:)는 View에서 발생한 사용자 입력에 따른 사이드 이펙트를 Store 외부로 전달하기 위한 메서드입니다.
내부 로직에서는 해당 액션을 sideEffectPublisher로 전파만 하며, 실제 처리(예: 화면 전환 등)는 View나 Coordinator(Router)에서 수행합니다.
이 구조를 통해 View는 UI 제어에 집중하고, Store는 상태와 사이드 이펙트 트리거만 책임지도록 역할을 분리했습니다.
| .onReceive(intent.sideEffectPublisher) { effect in | ||
| switch effect { | ||
| case .ui(.setMemoInputFocus): | ||
| isMemoInputFieldFocused = true | ||
| case .ui(.removeMemoInputFocus): | ||
| isMemoInputFieldFocused = false | ||
| case .navigation(.toComfieZoneSetting): | ||
| router.push(.comfieZoneSetting) | ||
| case .navigation(.toRetrospection(let memo)): | ||
| // TODO: 이후에 회고 뷰에 메모를 전달해줘야 한다. | ||
| router.push(.retrospection) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
View 관련 사이드 이펙트를 받아 UI 상태 변경 및 화면 전환을 View에서 직접 처리하도록 구성했습니다.
(ex: 포커스, 라우팅 등)
There was a problem hiding this comment.
포커스 관련 값 변경은 View의 시각적 상태에 대한 것이기 때문에 이 위치에서 처리하는 것이 자연스럽고 이해가 잘 됩니다. 추가로, 앞서 다른 코멘트에서 화면 전환이 View의 책임이 아닐까라고 언급한 부분을 확인했습니다.
하지만 화면 전환(navigation)은 Intent에 대한 action 혹은 side effect일 뿐, View의 직접적인 책임이라고 보긴 어려울 것 같습니다. View는 사용자의 effect만 전달하고, 그 결과로 나타나는 화면 전환은 Intent가 가져야 하지 않을까 생각합니다!
화면 전환은 View가 가지는 책임이라고 보기 어려울 것 같습니다. View는 사용자의 의도를 전달해줄 뿐, 의도를 처리하는 과정에서 발생하는 화면 전환은 오히려 Presentation 계층보다는 Domain에 가깝지 않을까 생각합니다. 현재는 UseCase와 같은 비즈니스 로직을 담당하는 객체가 없으니 Intent에서 처리하게 되겠지요..?
추가로 View가 가지는 화면 전환의 책임을 없애고 View 간의 의존성을 끊고자 View를 생성하는 ViewFactory(어쩌면 곧 DIContainer가 될 수도 있는?)를 사용했고, 화면 전환을 처리하는 Router를 구현했습니다. 이에 의도적으로 화면 전환을 View 바깥으로 분리하는 것이 옳다고 생각하는데.. 다들 어떻게 생각하는지 궁금합니다잉!!!
There was a problem hiding this comment.
사실 저도 처음에는 router를 누가 들고 있어야 할까? 라는 고민에서 시작했어요.
라우터는 "화면 전환"이라는 UI와 밀접한 행위를 수행하는 객체라고 느껴져서,
Store보다는 View가 소유하는 것이 더 자연스럽다고 생각했었습니다.
초기에는 기존 구조를 그대로 따라 Store에 router를 넣고 전체 로직을 처리했는데,
점점 Store의 책임이 많아지면서 UI까지 Store가 직접 다루는 게 과한 게 아닐까? 하는 고민이 생겼고,
이 과정에서 화면 전환은 View 쪽으로 분리하는 게 더 나은 선택이라고 판단했었습니다.
그런데 이번에 이오의 리뷰를 보면서 다시 생각해보게 되었습니다.
말씀처럼 ViewFactory의 목적이 View 간 의존성 제거였다면,
라우팅 로직도 View 밖으로 꺼내는 방향, 즉
Store (혹은 향후 UseCase)가 라우팅까지 책임지는 구조가 더 일관된 방향일 수 있겠다는 생각이 들었습니다.
지금 제 고민은 결국 이 지점인 것 같아요:
"화면 전환을 UI(Presentation)로 볼 것이냐, 아니면 도메인(Domain)에 가까운 흐름으로 볼 것이냐?"
아직은 어느 쪽이 더 맞는지 확신이 들진 않아서…
구리스의 의견까지 같이 들어보고 나서 팀적으로 합의된 방향으로 정리하면 좋을 것 같아요! 😄
There was a problem hiding this comment.
저도 이 부분에 대해 고민할 때마다 router는 시각적으로는 UI지만, 그 동기를 만든 건 비즈니스 흐름이 아닐까?’라는 생각을 했었어요. 예를 들어 어떤 버튼을 눌러서 '로그인이 완료되었으니 홈으로 이동해야 한다'는 판단은 결국 비즈니스 로직의 결과이고, 그 결과로 UI가 반응하는 느낌인거져...
그래서 저도 기본적으로는 View는 사용자 상호작용만 받고, 라우팅을 포함한 그 이후의 흐름은 Store나 UseCase 쪽에서 결정하는 구조가 더 일관되고 유지보수하기 좋다고 생각해요. 특히 라우팅이 복잡해질수록, View에 두면 View 간 결합도 생기고 테스트도 어려워질 수 있다고 생각합니다.
물론 View에서 직접적인 키보드 호출 같은 시각적 처리까지는 자연스러운 책임인 것 같고요.
결국 라우팅이 단순히 화면 이동인가, 아니면 흐름의 일부인가에 따라 책임이 달라질 수 있을 텐데, 지금 프로젝트에서는 도메인 흐름에 더 가까운 쪽으로 보고 Store (혹은 UseCase)에서 처리하는 쪽이 더 장기적인 구조 안정성에는 좋을 것 같아요!
답장이 늦어져 죄송합니닷 🙇♀️
| extension Date { | ||
| var hourAndMinuteString: String { | ||
| let formatter = DateFormatter() | ||
| formatter.dateFormat = "HH:mm" | ||
| return formatter.string(from: self) | ||
| } | ||
|
|
||
| var dotYMDFormat: String { | ||
| self.formatted(.dateTime.year().month(.twoDigits).day(.twoDigits)) | ||
| } | ||
| } |
There was a problem hiding this comment.
구리스도 비슷한 로직을 작성하신 걸로 보이는데,
이후에 해당 부분은 공통 로직으로 묶어서 한 곳에 합쳐도 좋을 것 같습니다 😊
There was a problem hiding this comment.
굿이요. 호랑 먼저 머지하면 제가 해당 파일에 합쳐서 올려야할 것 같네유
anjiniii
left a comment
anjiniii
left a comment
There was a problem hiding this comment.
아직 리뷰를 완료하지 못했지만, 우선 눈에 보이는 몇 가지 의견만 남겨보았습니다!
그리고 요건 기획 담당분들과 이야기를 해봐도 좋을 것 같은데요! 앱 진입 시, 스크롤이 가장 하단에 위치하여 최신 메모부터 보는 것은 어떨까요? 마치 채팅방에 들어왔을 때 최신 내용부터 보이는 것처럼요! ㅎㅎㅎ 그리고 관련된 내용으로 ㅎㅎ... 메모를 추가했을 때에도 스크롤이 가장 아래로 내려져 있으면 좋을 것 같습니다 하하...
| // TODO: MemoRepository를 어디서 주입할 지 고민하기 | ||
| let memoRepository: MemoRepositoryProtocol = MemoRepository() |
There was a problem hiding this comment.
기존에는 생성해야하는 것들이 화면/Intent 뿐이어서 View를 만든다는 의미의 ViewFactory로 명명했는데요! Repository 및 혹시 추후에 생길 수도 있는 다양한 의존성들의 주입을 이곳에서 진행하고, DIContainer 로 이름을 변경하는 것은 어떨지 제안해봅니다!
| init(coreDataService: CoreDataService = CoreDataService()) { | ||
| self.coreDataService = coreDataService | ||
| } |
There was a problem hiding this comment.
호랑이 Repository를 생성해주면서, 현재 모든 의존성을 상위에서 주입해주고 있습니다! 여기 Service도 상위에서 생성해서 Repository에 주입하는 건 어떤가요? 일관성과 유지보수 측면에서 더 좋아보이긴 하지만, 흠.. 저도 고민이 됩니다!
There was a problem hiding this comment.
DIContainer로 이름을 바꾸는 거 저도 찬성입니다!
(구리스도 의견 플리즈~🙉)
의존성을 주입해주는 곳을 한 곳에 작성해두는 것도 이후에 확인하기 더 좋을 거 같아요!
(ViewFactory에서 주입해주자는거 맞죠..?)
| .onReceive(intent.sideEffectPublisher) { effect in | ||
| switch effect { | ||
| case .ui(.setMemoInputFocus): | ||
| isMemoInputFieldFocused = true | ||
| case .ui(.removeMemoInputFocus): | ||
| isMemoInputFieldFocused = false | ||
| case .navigation(.toComfieZoneSetting): | ||
| router.push(.comfieZoneSetting) | ||
| case .navigation(.toRetrospection(let memo)): | ||
| // TODO: 이후에 회고 뷰에 메모를 전달해줘야 한다. | ||
| router.push(.retrospection) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
포커스 관련 값 변경은 View의 시각적 상태에 대한 것이기 때문에 이 위치에서 처리하는 것이 자연스럽고 이해가 잘 됩니다. 추가로, 앞서 다른 코멘트에서 화면 전환이 View의 책임이 아닐까라고 언급한 부분을 확인했습니다.
하지만 화면 전환(navigation)은 Intent에 대한 action 혹은 side effect일 뿐, View의 직접적인 책임이라고 보긴 어려울 것 같습니다. View는 사용자의 effect만 전달하고, 그 결과로 나타나는 화면 전환은 Intent가 가져야 하지 않을까 생각합니다!
화면 전환은 View가 가지는 책임이라고 보기 어려울 것 같습니다. View는 사용자의 의도를 전달해줄 뿐, 의도를 처리하는 과정에서 발생하는 화면 전환은 오히려 Presentation 계층보다는 Domain에 가깝지 않을까 생각합니다. 현재는 UseCase와 같은 비즈니스 로직을 담당하는 객체가 없으니 Intent에서 처리하게 되겠지요..?
추가로 View가 가지는 화면 전환의 책임을 없애고 View 간의 의존성을 끊고자 View를 생성하는 ViewFactory(어쩌면 곧 DIContainer가 될 수도 있는?)를 사용했고, 화면 전환을 처리하는 Router를 구현했습니다. 이에 의도적으로 화면 전환을 View 바깥으로 분리하는 것이 옳다고 생각하는데.. 다들 어떻게 생각하는지 궁금합니다잉!!!
| private var groupedMemos: [(date: String, memos: [Memo])] { | ||
| let grouped = Dictionary(grouping: intent.state.memos) { memo in | ||
| memo.createdAt.dotYMDFormat | ||
| } | ||
| return grouped | ||
| .sorted { $0.key < $1.key } | ||
| .map { (key, value) in (date: key, memos: value) } | ||
| } |
There was a problem hiding this comment.
그룹화하고 정렬하는 과정을 Intent에서 처리하는 것은 어떨까요? 세부 내용을 모두 처리하고 groupdedMemos를 State로 활용할 수 있지 않을까 생각합니다!
There was a problem hiding this comment.
해당 groupedMemos는 View에서만 사용하는 UI 표현용 데이터라
화면 구성 책임은 View에 있다고 생각해 뷰 내부에 선언했습니다!
정렬/그룹화도 단순한 로직이라 Store까지 가져가지 않아도 된다고 판단했지만,
View를 최대한 껍데기처럼 두고 싶다면 Store로 옮기는 것도 충분히 가능하다고 생각합니다!
| // MARK: Intent | ||
| enum Intent { | ||
| case showRetrospectionView // 회고 화면으로 | ||
| case showComfieZoneSettingView // 컴피존 설정 화면으로 | ||
| case deletePopup(PopupIntent) | ||
| case memoInput(MemoInputIntent) | ||
| case memoCell(MemoCellIntent) | ||
|
|
||
| case onAppear | ||
| case backgroundTapped | ||
| case comfieZoneSettingButtonTapped | ||
|
|
||
| enum PopupIntent { | ||
| case confirmDeleteButtonTapped | ||
| case cancelDeleteButtonTapped | ||
| } | ||
|
|
||
| enum MemoInputIntent { | ||
| case updateNewMemo(String) | ||
| case memoInputButtonTapped | ||
| } | ||
|
|
||
| enum MemoCellIntent { | ||
| case editButtonTapped(Memo) | ||
| case retrospectionButtonTapped(Memo) | ||
| case deleteButtonTapped(Memo) | ||
| case editingCancelButtonTapped | ||
| } | ||
| } |
There was a problem hiding this comment.
네! 확실히 Intent의 내용이 많아지니 한눈에 읽히기 어려운 것 같네요! Cell이 맡고 있는 의도가 많다면 분리하는 것도 좋아보입니다.
| // TODO: MemoRepository를 어디서 주입할 지 고민하기 | ||
| let memoRepository: MemoRepositoryProtocol = MemoRepository() |
There was a problem hiding this comment.
오호랏 전 이 ViewFactory가 팩토리 메소드 패턴을 채택한 아이라고 생각해서 repository와 같은 인스턴스들을 주입해도 된다고 생각했습니다! +) 네이밍은 크으게 상관 없어보이는데 바꿔도 의미 상 틀린 건 없어서 좋습니당
| // MARK: Intent | ||
| enum Intent { | ||
| case showRetrospectionView // 회고 화면으로 | ||
| case showComfieZoneSettingView // 컴피존 설정 화면으로 | ||
| case deletePopup(PopupIntent) | ||
| case memoInput(MemoInputIntent) | ||
| case memoCell(MemoCellIntent) | ||
|
|
||
| case onAppear | ||
| case backgroundTapped | ||
| case comfieZoneSettingButtonTapped | ||
|
|
||
| enum PopupIntent { | ||
| case confirmDeleteButtonTapped | ||
| case cancelDeleteButtonTapped | ||
| } | ||
|
|
||
| enum MemoInputIntent { | ||
| case updateNewMemo(String) | ||
| case memoInputButtonTapped | ||
| } | ||
|
|
||
| enum MemoCellIntent { | ||
| case editButtonTapped(Memo) | ||
| case retrospectionButtonTapped(Memo) | ||
| case deleteButtonTapped(Memo) | ||
| case editingCancelButtonTapped | ||
| } | ||
| } |
| .onReceive(intent.sideEffectPublisher) { effect in | ||
| switch effect { | ||
| case .ui(.setMemoInputFocus): | ||
| isMemoInputFieldFocused = true | ||
| case .ui(.removeMemoInputFocus): | ||
| isMemoInputFieldFocused = false | ||
| case .navigation(.toComfieZoneSetting): | ||
| router.push(.comfieZoneSetting) | ||
| case .navigation(.toRetrospection(let memo)): | ||
| // TODO: 이후에 회고 뷰에 메모를 전달해줘야 한다. | ||
| router.push(.retrospection) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
저도 이 부분에 대해 고민할 때마다 router는 시각적으로는 UI지만, 그 동기를 만든 건 비즈니스 흐름이 아닐까?’라는 생각을 했었어요. 예를 들어 어떤 버튼을 눌러서 '로그인이 완료되었으니 홈으로 이동해야 한다'는 판단은 결국 비즈니스 로직의 결과이고, 그 결과로 UI가 반응하는 느낌인거져...
그래서 저도 기본적으로는 View는 사용자 상호작용만 받고, 라우팅을 포함한 그 이후의 흐름은 Store나 UseCase 쪽에서 결정하는 구조가 더 일관되고 유지보수하기 좋다고 생각해요. 특히 라우팅이 복잡해질수록, View에 두면 View 간 결합도 생기고 테스트도 어려워질 수 있다고 생각합니다.
물론 View에서 직접적인 키보드 호출 같은 시각적 처리까지는 자연스러운 책임인 것 같고요.
결국 라우팅이 단순히 화면 이동인가, 아니면 흐름의 일부인가에 따라 책임이 달라질 수 있을 텐데, 지금 프로젝트에서는 도메인 흐름에 더 가까운 쪽으로 보고 Store (혹은 UseCase)에서 처리하는 쪽이 더 장기적인 구조 안정성에는 좋을 것 같아요!
답장이 늦어져 죄송합니닷 🙇♀️
- 팀 내 논의에 따라 router를 View가 아닌 Store 내부로 이동 - 화면 전환을 액션으로 분리하여 흐름 제어를 명확히 함
- 역할이 View 생성에만 국한되지 않고 의존성 주입 전반을 담당하게 되어 DIContainer로 네이밍 변경
About this PR
🔖 Related Issue
📚 Contents
📸 Screenshot