-
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
[#141] 속한 그룹이 없는 경우 그룹 생성 페이지로 이동 #164
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.
확실히 overview 에 잘 정리해주어서 코드리뷰할때 편하네~ 굿굿
네이밍 관련한 의견과 조금의 코멘트를 남겨보았으니 확인 부탁해!
...ntation/src/main/java/com/mashup/gabbangzip/sharedalbum/presentation/ui/main/MainActivity.kt
Outdated
Show resolved
Hide resolved
} | ||
viewModelScope.launch { | ||
getGroupListUseCase() | ||
.onSuccess { |
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.
Result 객체인만큼 onSuccess 뿐만아니라 onFailure 일때의 처리도 필요할 것 같아!
이대로면 그룹리스트 가져올때 네트워크 오류로 실패나면 무한로딩 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.
고생했어 지혜 ~~~계원오빠 코멘트 확인해주면 될 것 같당 난 일단 어푸 누를게?
...ashup/gabbangzip/sharedalbum/presentation/ui/groupcreation/intro/GroupCreationIntroScreen.kt
Show resolved
Hide resolved
...ntation/src/main/java/com/mashup/gabbangzip/sharedalbum/presentation/ui/main/MainActivity.kt
Outdated
Show resolved
Hide resolved
- LocalDateTime 변환 유틸 작성
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.
뭔가 서버와 nullable 하게 내려오는 데이터들에 대해서 swagger 등의 문서에 기재가 필요할 것 같다고 이야기가 필요할지도..?
일단 현재 수정된 내용은 좋다고 생각해서 어푸할게!!
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.
지혜 고생했엉~~~~ 나나 코멘트 확인만 해줠~~~
when (state) { | ||
is GroupHomeUiState.NoGroup -> { | ||
navigateToGroupCreationAndFinish() | ||
} | ||
|
||
is GroupHomeUiState.GroupList -> { | ||
GroupHomeScreen( |
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.
나아는 개인적으로 이거 일관성을 맞췄으면 좋겠엉... 우리가 지금 state는 dataclass로 사용하고 있는데 어느거는 dataclass 고 어느거는 sealed로 하는거를 선호하지 않기는해..!
이 부분 같은 경우도 noGroup아니면 groupList인데 충분히 emptyList나 null로 처리할 수 있는거 처럼 보여..!
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.
이게 초기값이 emptyList라서 무조건 그룹생성으로가더라고.... 그렇다고 nullable로 하자니 너무 귀찮아지는 것 같았는데 이게 더 귀찮은가 근데 nullable로 해도 초기값이 null이면 무조건 그룹 생성으로 가지 않을까? 그러면 isInitialized 이런거 하나 넣을까?
@@ -67,7 +69,39 @@ import com.mashup.gabbangzip.sharedalbum.presentation.utils.noRippleClickable | |||
|
|||
@Composable | |||
fun GroupHomeScreen( |
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.
GroupHomeScreeen을 두개로 만든이유가 람다가 많아서 그런건가..? 아니면 하나의 스크린에서 if문을 통해 처리하면 홈화면이 한번 나오고 그룹만들기 화면으로 가서 그런걸까..? 나아는 GroupHomeScreeen을 오버로딩 하는게 마음에 살짝 걸려서..! 차라리 GroupHomeScreen을 두개로 두는 거보다 GroupHomeContent라던지 다른 이름이 낫지 않을까 싶어서..! (아님 말구..ㅎ)
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.
GroupHomeViewModel을 받아서 state를 구독하는 stateful screen이랑 stateless screen으로 분리했어!
처음에는 MainViewModel에 그룹조회 유스케이스를 넣을까 하다가 그렇게 되면 MainNavHost에 그룹 조회 state를 넘겨줘야 하는데 이게 뭔가 다른 스크린에서는 불필요한 정보라고 생각되더라구
이렇게 두개를 두면 다른 컴포저블에 GroupHomeUiState를 넘기지 않을 수도 있고, stateless 스크린으로 스크린 프리뷰도 볼수있어서 깔끔하지 않나 생각해
Issue No
Overview (Required)
onClick~
는 어떤 행위 또는 원인을 의미하고navigateTo~
는 그로 인한 결과인데 주로 첫번째걸로 네이밍을 해왔단말이죠,,? 그래서 저번 PR에도 스낵바가 필요해! 와 스낵바를 보여줘! 중에 고민을 했었고, 이번에도그룹 생성이 필요해!
와그룹 생성으로 이동하고 현재화면 닫아줘!
중에 고민을 하다가 다른 친구들이 다 onClick이길래 비슷한 느낌으로다가 그룹 생성이 필요해로 이름을 지었는데 음... 어떻게 생각하시는지?+++ 하는김에 이것저것 오류 수정..