[Session, Bookmark] 세션 상세 화면에서 북마크 변경시 UI 표시 #234
Conversation
gowoon-choi
changed the title
...designsystem/src/main/java/com/droidknights/app2023/core/designsystem/component/TopAppBar.kt
Show resolved
Hide resolved
...on/src/main/java/com/droidknights/app2023/feature/session/SessionDetailBookmarkStatePopup.kt
Outdated
Show resolved
Hide resolved
...on/src/main/java/com/droidknights/app2023/feature/session/SessionDetailBookmarkStatePopup.kt
Outdated
Show resolved
Hide resolved
...on/src/main/java/com/droidknights/app2023/feature/session/SessionDetailBookmarkStatePopup.kt
Outdated
Show resolved
Hide resolved
| @@ -51,41 +57,75 @@ internal fun SessionDetailScreen( | |||
| ) { | |||
| val scrollState = rememberScrollState() | |||
| val sessionUiState by viewModel.sessionUiState.collectAsStateWithLifecycle() | |||
| var bookmarkState by remember { mutableStateOf(false) } | |||
| val effect by viewModel.sessionUiEffect.collectAsStateWithLifecycle(SessionDetailEffect.Idle) | |||
There was a problem hiding this comment.
기본값이 필요하다면 Channel.receiveAsFlow가 아닌 StateFlow를 고려해볼 수도 있을 것 같은데요, Channel을 사용하신 이유가 궁금해요!
There was a problem hiding this comment.
처음에는 팝업을 지우는 조건이나 상황을 고려하지 않고, 띄우는 것만 생각하고 구성을 진행해서 초기값이 없고, ShowToastForBookmarkState가 연속하여 들어올 수도 있다고 생각하고 구성을 진행하여서 Channel로 구성하였습니다.
요구사항과 구현내용을 꼼꼼하게 파악하지 못하고 작업부터 진행하다보니 중간에 구성이 바뀌게 되면서 Channel로 구성하지 않아도 되는 상황이 되었는데 미처 파악을 못 했네요.
리뷰 감사합니다 ~! 반영하겠습니다 !
수정 진행하면서 한 가지 고민되는 부분이 있어 조언 구합니다 !
기존에는 Channel로 구현하여 effect라는 Flow를 별도로 둔 이유도 있었지만, ShowToast가 bookmark 상태 업데이트에 따른 부수적인 UI 효과이므로 effect라는 별도의 flow로 관리를 해주는 것이 좋겠다는 생각도 있었습니다.
하지만 팝업 노출 여부도 결국은 UI 상태 중에 하나라는 생각이 들어 StateFlow로 변경하면서 SessionDetailUiState.Success에 isShowPopup 이라는 값을 하나 추가할까 싶은 생각이 들었습니다.
혹시 관련하여 @wisemuji 님이라면 두 가지 방식중에, 혹은 다른 방식이라도 어떻게 구현을 하실지, 이유는 무엇일지 조언 구하고 싶습니다 ! :)
There was a problem hiding this comment.
@gowoon-choi
SessionDetailUiState 내에서 관리할 수도 있겠으나, 팝업 상태를 정의하는 값이 변경이 자주 일어날 수 있는 값임도 고려해야 합니다. 팝업 상태를 정의하는 값이 변경될때마다 SessionDetailUiState가 매번 수정되는 것이 불필요한 수정이라고 여겨질 수도 있기 떄문이에요. 이 관점에 대해서는 정답이 있기보다, 각각의 장단점을 비교해보고 개발자가 선택하는 것이 맞다고 생각합니다 🙂
주관적으로는 SessionDetailUiState와 따로 값을 관리하는 것을 선호하긴 합니다.
There was a problem hiding this comment.
UI 상태를 토스트로 출력할지, 다이얼로그로 출력할지, 스크린으로 출력할지 등은 UI의 관심사로 볼 수 있습니다.
그렇기 때문에 UI 상태에서는 어떻게 그릴지는 관심 없이 상태에만 집중 하는 것이 권장됩니다.
https://developer.android.com/jetpack/guide/ui-layer/events?hl=ko#consuming-trigger-updates
| @@ -51,41 +57,75 @@ internal fun SessionDetailScreen( | |||
| ) { | |||
| val scrollState = rememberScrollState() | |||
| val sessionUiState by viewModel.sessionUiState.collectAsStateWithLifecycle() | |||
| var bookmarkState by remember { mutableStateOf(false) } | |||
There was a problem hiding this comment.
북마크의 상태는 뷰모델에서 가지고 있는 재료로 판단할 수 있지 않을까요?
이곳에 Stateful한 북마크를 따로 가지는 목적이 있을까요?
There was a problem hiding this comment.
맞습니다 :) SessionDetailUiState의 북마크 값으로 판단하도록, 상태가 Success인 경우는 bookmarked 값으로 그 외의 경우는 false로 처리되도록 수정하였습니다 ! 감사합니다 !
Issue
Overview (Required)
Links
Screenshot