Week7 필수과제 구현 #12
Week7 필수과제 구현 #12
Conversation
angryPodo
requested review from
a team,
chattymin,
tunaunnie,
jihyunniiii and
t1nm1ksun
and removed request for
a team
| private val _effect = Channel<SignInEffect>() | ||
| val effect = _effect.receiveAsFlow() |
There was a problem hiding this comment.
Channel도 좋고 SharedFlow도 좋아요.
그런데 변경한 이유가 있을까요?
| enabled = !state.isLoading && | ||
| state.username.isNotBlank() && | ||
| state.password.isNotBlank() && | ||
| state.hobby.isNotBlank() |
There was a problem hiding this comment.
이 부분도 state에 넣는 것도 좋을 것 같네요
| if (state.isLoading) { | ||
| CircularProgressIndicator( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .wrapContentSize(), | ||
| color = Color.White | ||
| ) | ||
| } else { | ||
| LazyColumn( | ||
| modifier = Modifier.fillMaxSize() | ||
| ) { | ||
| item { | ||
| BannerView(state.bannerImages) | ||
| } | ||
|
|
||
| item { | ||
| SectionTitle("믿고 보는 웨이브 에디터 추천작") | ||
| EditorPicksList(uiState.editorPicks) | ||
| } | ||
| item { | ||
| SectionTitle("믿고 보는 웨이브 에디터 추천작") | ||
| EditorPicksList(state.editorPicks) | ||
| } | ||
|
|
||
| item { | ||
| SectionTitle("오늘의 TOP 20") | ||
| Top20List(uiState.top20Items) | ||
| item { | ||
| SectionTitle("오늘의 TOP 20") | ||
| Top20List(state.top20Items) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드에서 LazyColumn을 사용하신 이유가 있나요?
LazyColumn을 사용했을 때의 이점이 없는 것 처럼 느껴져요.
단순히 스크롤을 넣고 싶으셨다면 Column에 scrollState를 넣는 방법도 있습니다.
| val bannerImages: List<Int> = emptyList(), | ||
| val editorPicks: List<String> = emptyList(), | ||
| val top20Items: List<String> = emptyList(), |
There was a problem hiding this comment.
List는 불변이 아닌, 읽기 전용입니다.
그렇기에 리컴포지션의 대상이 돼요.
@stable, @immutable과 같은 어노테이션을 활용하거나, ImmutableList를 활용해보시는 것도 리컴포지션을 줄이는데 도움이 될거에요
| package org.sopt.and.presentation.auth.signin | ||
|
|
||
| sealed interface SignInEffect { | ||
| data class ShowError(val message: String) : SignInEffect |
There was a problem hiding this comment.
저는 로그인 성공 시에 '로그인에 성공했습니다' 라는 문구를 띄워줘서 이 이벤트 이름을 showSnackbar로 했는데, 민재님 코드는 보니까 로그인 실패 시에 unknown user 라는 에러 메시지만 출력해주고 계시네요!! 클래스 이름에서부터 그 동작을 직관적으로 유추할 수 있어서 좋은 것 같아요
| @@ -18,45 +20,47 @@ import javax.inject.Inject | |||
| class SignInViewModel @Inject constructor( | |||
| private val signInUseCase: SignInUseCase | |||
| ) : ViewModel() { | |||
| private val _uiState = MutableStateFlow(SignInUiState()) | |||
| val uiState: StateFlow<SignInUiState> = _uiState.asStateFlow() | |||
| private val _state = MutableStateFlow(SignInState()) | |||
There was a problem hiding this comment.
저는 워크북에 있는 파트장님의 코드를 참고해서 baseViewModel에 setState 함수를 정의해두고 사용했는데, 이렇게 하는 방법도 있군요!
| is SignUpIntent.TogglePasswordVisibility -> togglePasswordVisibility() | ||
| is SignUpIntent.SignUp -> signUp() | ||
| } | ||
| } |
There was a problem hiding this comment.
뭔가 제 코드보다 훨씬 읽었을 때 이해가 잘 되는 것 같아요 ...!! Contract 안에 Effect, Event(Intent), State를 다 넣어두지 않고 하나씩의 분리된 파일로 만들면 이런 코드 쓸 때 훨씬 가독성이 좋아지는 것 같아요
| package org.sopt.and.presentation.auth.signin | ||
|
|
||
| sealed interface SignInIntent { | ||
| data class UpdateUsername(val username: String) : SignInIntent | ||
| data class UpdatePassword(val password: String) : SignInIntent | ||
| data object TogglePasswordVisibility : SignInIntent | ||
| data object SignIn : SignInIntent | ||
| } |
There was a problem hiding this comment.
세미나 자료처럼 Effect,Intent,State를 Contract에 작성하면 파일개수 줄일 수 있을거 같아요
| @@ -1,17 +1,21 @@ | |||
| package org.sopt.and.presentation.home | |||
|
|
|||
| import android.app.Activity | |||
| import android.widget.Toast | |||
| import androidx.activity.compose.BackHandler | |||
| import androidx.compose.foundation.background | |||
| import androidx.compose.foundation.layout.* | |||
| enabled = !state.isLoading && | ||
| state.username.isNotBlank() && | ||
| state.password.isNotBlank() |
There was a problem hiding this comment.
얘 자체를 상태로 관리해도 좋을 듯 하네요!
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
완벽한 MVI를 적용한건 아니지만,,그래도 컨셉은 지키도록 열심히 해봤습니다. 많은 지적 부탁드립니다!