-
Notifications
You must be signed in to change notification settings - Fork 3
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
[AN] 스터디 개설 뷰 리펙터링 #618
base: develop
Are you sure you want to change the base?
[AN] 스터디 개설 뷰 리펙터링 #618
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.
Life Good TM
} | ||
private fun collectCreateStudyState() { | ||
createStudyViewModel.createStudyState | ||
.collectLatestOnStarted(this) { createStudyState -> |
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.
왜 collectLatest를 사용하셨나요?
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에 Loading과 Idle을 넣기 전에 중복 시도 시 이전 통신 값은 무시하고 마지막에 요청한거만 하도록 하려고 이렇게 했었슴다
지금은 collect든 collectLatest든 둘 다 동일한 동작을 수행할 거 같네용
@@ -112,15 +115,34 @@ class CreateStudyViewModel @Inject constructor( | |||
) | |||
createStudyRepository.createStudy(study) | |||
.onSuccess { studyId -> | |||
_createStudyUiState.emit(CreateStudyUiState.Success(studyId)) | |||
_createStudyEvent.emit(Event.CreateStudySuccess) | |||
_createStudyState.emit(CreateStudyState.Success(studyId)) |
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.
세터말고 emit한 이유가 뭔가요?
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 vs setter vs emit
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.
setter랑 emit 차이는 없는걸로 알고 있었는데 update는 첨보네요 굳
else -> { | ||
null | ||
} | ||
TAG_CREATE_STUDY_PEOPLE_COUNT -> PeopleCountBottomSheetFragment() |
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.
고생하셨습니다. 써늬
몇가지 코멘트 남겼으니
읽어보시고 잇더즌메잌센스하다 싶은건 걸러주세요!
sealed interface Event { | ||
object CreateStudySuccess : Event | ||
object CreateStudyFailure : Event | ||
data class NavigateToBefore(val fragmentState: FragmentState) : Event | ||
data class NavigateToNext(val fragmentState: FragmentState) : Event | ||
} | ||
|
||
sealed interface CreateStudyState { | ||
class Success(val studyId: Long) : CreateStudyState | ||
|
||
object Loading : CreateStudyState | ||
|
||
object Failure : CreateStudyState | ||
|
||
object Idle : CreateStudyState | ||
} |
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
에서는 어느 state인지 CrateStudyState
라고 해주셨는데,
위의 Event
는 그냥 Event라고 명시해주셨네요.
서로 다른 이유가 혹시 있을까요?
binding.onInputClickListener = ::setOnInputClick | ||
private fun setupPeopleCount() { | ||
createStudyViewModel.peopleCount.collectOnStarted(viewLifecycleOwner) { peopleCount -> | ||
if (peopleCount == -1) return@collectOnStarted |
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.
-1은 무엇을 뜻하나용?
val isEnableFirstCreateStudyNext: StateFlow<Boolean> = | ||
combine(peopleCount, studyDate, cycle) { peopleCount, studyDate, cycle -> | ||
return@combine peopleCount != DEFAULT_INT_VALUE && studyDate != DEFAULT_INT_VALUE && cycle != DEFAULT_INT_VALUE | ||
} | ||
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(), false) | ||
|
||
val isEnableSecondCreateStudyNext: Flow<Boolean> = | ||
val isEnableSecondCreateStudyNext: StateFlow<Boolean> = | ||
combine(studyName, studyIntroduction) { studyName, studyIntroduction -> | ||
return@combine studyName.isNotBlankAndEmpty() && studyIntroduction.isNotBlankAndEmpty() | ||
} | ||
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(), false) |
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.
혹시 combine 블록 내부에
return@combine
구문이 필요한가요??
블럭 내부에 가장 마지막 줄이 return의 역할을 하지 않나요?
제가 놓치는 점이 있다면 꼭 말씀주세요! 궁금합니다.
'블록의 마지막 식이 블록의 결과'라는 규칙은 블록이 값을 만들어내야 하는 경우 항상 성립한다. - Kotlin in action
sealed interface Event { | ||
object CreateStudySuccess : Event | ||
object CreateStudyFailure : Event | ||
data class NavigateToBefore(val fragmentState: FragmentState) : Event | ||
data class NavigateToNext(val fragmentState: FragmentState) : Event | ||
} |
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.
제가 참고했던 구글의 프로젝트에선 Event가 아닌 Action으로 정의하고
아래 처럼 Action(행동)에 관한 Navigate관련 클래스 두개로만 관리하긴 합니다.
sealed class OnboardingNavigationAction {
object NavigateToMainScreen : OnboardingNavigationAction()
object NavigateToSignInDialog : OnboardingNavigationAction()
}
현재 코드는 행동과 이벤트(상황)가 혼합된 느낌이긴 합니다.
분리하는 것에 대해 어떻게 생각하시나요?
😋 작업한 내용
🙏 PR Point
👍 관련 이슈