[Feat] Login 영역에서의 ProgressBar 추가 #285
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (3)
- app/src/main/AndroidManifest.xml: Language not supported
- app/src/main/res/layout/activity_login.xml: Language not supported
- app/src/main/res/values/strings.xml: Language not supported
Comments suppressed due to low confidence (3)
app/src/main/java/com/eatssu/android/presentation/login/LoginViewModel.kt:51
- The change from collectLatest to collect may lead to processing outdated emissions if multiple events occur rapidly. Please confirm that sequential processing without cancellation is the intended behavior.
.collect { result ->
app/src/main/java/com/eatssu/android/presentation/login/LoginViewModel.kt:42
- [nitpick] The function name 'getKakaoLogin' suggests a getter behavior, while it actually triggers a login operation. Consider renaming it to 'loginWithKakao' for improved clarity.
fun getKakaoLogin(email: String, providerID: String) {
app/src/main/java/com/eatssu/android/presentation/login/LoginActivity.kt:56
- Since the loading state is managed via the state observer, directly calling showLoading in the login flow may result in conflicting UI updates. Consider relying solely on observeState for managing loading visibility.
showLoading(true) // ProgressBar 표시
| private fun handleKakaoLogin() { | ||
| lifecycleScope.launch { | ||
| try { | ||
| showLoading(true) // ProgressBar 표시 |
There was a problem hiding this comment.
근데 state값이 loading일때만 보여줘야 하는 거 아닌가요?여기는 왜 프로그레스바가 true인가요
There was a problem hiding this comment.
영상 보면 로딩바가 두번 나오잖아요?
- 로그인 버튼 클릭하고 바로 [카카오 sdk에서 회원정보 받아오기]
- 카카오 동의 하고 이후 ~ 메인 넘어가기 전 [1에서 받아온 정보를 바탕으로 우리 rest api 호출]
두 단계로 나눠져서 두번 뜨는거에요!
1번이 handleKakaoLogin함수 로그인 입니당 그래서 1번 과정 중에 로딩바 띄우려면 저기가 True여야 해요! finally에서 false로 해제하는 과정까지가 1번 과정입니당
There was a problem hiding this comment.
아 넵 ! 순서는 이해했아여
그러면 여기서 직접 showLoading(true) 를 하는 것 보다
uistate를 loading으로 업데이트하도록 (어차피 uistate가 laoding으로 되면 showLoading(true)을 호출하도록 관찰중이기 때문에)
뷰모델에서 uistate를 업데이트하는 함수를 만들고 그거를 호출하는 방식은 어떨까 해서요!
정리하자면
- 액티비티에서는 uistate 업데이트 하는 뷰모델 함수 호출
- 뷰모델에서는 uistate 업데이트 되어 전체적으로 loading 되는 uistate가 됨
- uistate에 따라서 로딩 화면이 보여짐
이 과정이 좀 더 안정적이고 uistate가 통일성이 있지 않을까 합니다
이 과정이 없다면 uistate는 init인데 보여지는 화면은 로딩이니까요!
There was a problem hiding this comment.
오... 그러면 단방향 데이터 흐름 원칙에 위배되지 않나요? 뷰는 uistate에 따라서 보여주기만 하는 역할인데, 굳이 무리해서 뷰->뷰모델로 uistate 전달을 해야한다는거에 공감을 못하겠어요...
(추가로 드는 생각은 첫번째 나타나는 로딩은 api 통신이 아니라 sdk 작업이라서 uistate loading이 아니어도 괜찮을 것 같아요. api 통신이 시작하지 않은거니까요! (유리님이 말씀하신대로 했을 때) 첫번째 로딩 중에 뒤로가기를 누르거나 중단하게 되면(+네트워크 오류 시) 계속 loading 상태에 갇혀버리지 않을까요? loading 상태에서는 버튼이 없으니까 사용자가 취할 수 있는 액션이 없죠. 그래서 저는 첫번째 과정이 uistate.loading이 아니어도 되지 않을까..? 생각합니다!)
There was a problem hiding this comment.
오 아니죠 오히려 저는 단방향 데이터 흐름을 지키자고 제안 드린 방법입니다
지금처럼 Activity에서 직접 showLoading(true)을 호출하는 건,
상태가 아닌 '행위'로 UI를 제어하는 거라 오히려 단방향 흐름이 깨지지 않을까요
ViewModel이 상태를 관리하고,
그 상태에 따라 UI가 반응하는 구조가 MVVM + 단방향 흐름으로 알고 있어서요
UI는 상태에만 의존하게 되는거죠
말씀드린 부분으로 수정하게 되면 UI → ViewModel → State → UI 흐름을 따르게 됩니다
단방향 흐름이 깨진다는 의견에는 위의 답변 드리고
sdk 작업이니 uistate를 따르지 않아도 된다고 생각하시면... 기존대로 하셔도 되긴 하지만
가능하면 ViewModel에서 uiState로 통합적으로 관리하는 게 더 안정적이지 않을까 싶어요.
사실 둘다 문제가 되는 코드는 아니니.. 제 마지막 의견 들어보시고 자유롭게 해주세욤
There was a problem hiding this comment.
뷰->뷰모델로 uistate "전달" 하는 게 아니라 뷰모델에서 uistate를 업데이트 하라고 (api 호출하는 함수를 사용하듯) 이야기하는 거라고 생각합니다
API 호출 함수를 사용하듯이 ViewModel의 상태 업데이트 함수를 호출하자는 의미였습니다.
There was a problem hiding this comment.
오키 납득!!!!! 뷰가 state에 따라서 이에 적절한 ui를 뿌려주는게 맞는 것 같습니다. 아이가릿~~~~~
| loginViewModel.uiState.collect { state -> | ||
| when (state) { | ||
| is LoginState.Loading -> showLoading(true) | ||
| is LoginState.Success -> {} |
There was a problem hiding this comment.
요기 showLoading(false) 하면 되지 않을까요?
|
@kangyuri1114 2번 과정에서 카카오로그인 버튼이 안나오게 하기는 안되네요.. 한가지 고민인거는 모델링에 대한건데, 로그인의 경우 성공시 UI한테 전달해줄 데이터가 없습니다. 이런 경우에는 LoginState 모델링을 어떻게 하는게 좋을지 모르겠어요.. |
kangyuri1114
left a comment
kangyuri1114
left a comment
There was a problem hiding this comment.
리뷰 한번 더 확인부탁드려요! 수고하셨습니당 👍
| private fun handleKakaoLogin() { | ||
| lifecycleScope.launch { | ||
| try { | ||
| showLoading(true) // ProgressBar 표시 |
There was a problem hiding this comment.
아 넵 ! 순서는 이해했아여
그러면 여기서 직접 showLoading(true) 를 하는 것 보다
uistate를 loading으로 업데이트하도록 (어차피 uistate가 laoding으로 되면 showLoading(true)을 호출하도록 관찰중이기 때문에)
뷰모델에서 uistate를 업데이트하는 함수를 만들고 그거를 호출하는 방식은 어떨까 해서요!
정리하자면
- 액티비티에서는 uistate 업데이트 하는 뷰모델 함수 호출
- 뷰모델에서는 uistate 업데이트 되어 전체적으로 loading 되는 uistate가 됨
- uistate에 따라서 로딩 화면이 보여짐
이 과정이 좀 더 안정적이고 uistate가 통일성이 있지 않을까 합니다
이 과정이 없다면 uistate는 init인데 보여지는 화면은 로딩이니까요!
indeterminateTint
HI-JIN2
force-pushed
the
feat/loading-bar-login
branch
from
9eff94e to
bbdf01f
Compare
default.webm@kangyuri1114 최종 처리해두었습니다! 뷰에서 뷰모델의 메소드를 통해서 uiState를 제어할 수 있도록 하였고, 제가 우려했던 시나리오(1번 과정에서의 이탈)시 다시 init으로 돌아가서 로그인 버튼 띄우도록 했습니다! 그리고 Success에서는 showLoading(false) 안하고 그냥 화면 넘기는 코드만 넣어뒀어요. showLoading(false) 하니까 화면 전환 직전에 로그인 버튼이 다시 나타나서..! 그리고 토큰 valid 하는 api 만들어져서 해당 처리(관련 메소드 파일 생성.. 등) 도 해두었습니다! 9a1388b |
Summary
지금 서버가 느려서 api 호출 응답 자체가 느린데, 거기에 추가로 로딩바가 없어서 더 느리고 불안정하게 느껴집니다. 그래서 로딩바를 넣어봤는데 한번 봐주세요!
api 호출 하나당 길게는 10초 정도 걸릴 때가 있어요. 특히 맨 처음 api 호출의 경우 로그인이기 때문에 로딩바가 없을때에 그냥 동작을 아예 안하는것처럼 보이기도 합니다. 그래서 에타에서 그런 반응이 나왔던 것 같아요. 제가 쓰기에도 좀 Something to Wrong 하게 보이더라구요
아무튼. 카카오로그인 버튼 있는 쪽에 로딩바(동그란거) 넣어봤는데 괜찮은지 한번 봐주세요!
Describe your changes
default.webm
로그인시 로딩바가 두번 나옵니다
위치가 좀 애매하다 색깔이 너무 연하다 이런 코멘트도 좋습니다.
To reviewers