-
Notifications
You must be signed in to change notification settings - Fork 0
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
Week7 필수과제 구현 #12
base: develop
Are you sure you want to change the base?
Week7 필수과제 구현 #12
Conversation
private val _effect = Channel<SignInEffect>() | ||
val effect = _effect.receiveAsFlow() |
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.
Channel
도 좋고 SharedFlow
도 좋아요.
그런데 변경한 이유가 있을까요?
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.
이번기수 고생하셨습니다! 민재님 덕분에 재미있게 코드리뷰 작성했습니다 :)
앱잼 화이팅이에요 🚀
enabled = !state.isLoading && | ||
state.username.isNotBlank() && | ||
state.password.isNotBlank() && | ||
state.hobby.isNotBlank() |
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에 넣는 것도 좋을 것 같네요
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드에서 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List는 불변이 아닌, 읽기 전용입니다.
그렇기에 리컴포지션의 대상이 돼요.
@stable, @immutable과 같은 어노테이션을 활용하거나, ImmutableList
를 활용해보시는 것도 리컴포지션을 줄이는데 도움이 될거에요
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.
열심히 하신 노력이 다 드러나는 코드인 것 같습니다..!! 시험 기간이라 많이 바쁘셨을 것 같은데 고생 많으셨습니다!!
package org.sopt.and.presentation.auth.signin | ||
|
||
sealed interface SignInEffect { | ||
data class ShowError(val message: String) : SignInEffect |
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.
저는 로그인 성공 시에 '로그인에 성공했습니다' 라는 문구를 띄워줘서 이 이벤트 이름을 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 워크북에 있는 파트장님의 코드를 참고해서 baseViewModel에 setState 함수를 정의해두고 사용했는데, 이렇게 하는 방법도 있군요!
is SignUpIntent.TogglePasswordVisibility -> togglePasswordVisibility() | ||
is SignUpIntent.SignUp -> signUp() | ||
} | ||
} |
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.
뭔가 제 코드보다 훨씬 읽었을 때 이해가 잘 되는 것 같아요 ...!! Contract 안에 Effect, Event(Intent), State를 다 넣어두지 않고 하나씩의 분리된 파일로 만들면 이런 코드 쓸 때 훨씬 가독성이 좋아지는 것 같아요
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.
졸업이슈로 난 과제 못했는데 야무지게 잘했다!!! (진짜임 달게 없어 나보다 잘하는듯)
앱잼화이팅 행복해야해!!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세미나 자료처럼 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.* |
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.
고생하셨습니다 ~
enabled = !state.isLoading && | ||
state.username.isNotBlank() && | ||
state.password.isNotBlank() |
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.
얘 자체를 상태로 관리해도 좋을 듯 하네요!
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
완벽한 MVI를 적용한건 아니지만,,그래도 컨셉은 지키도록 열심히 해봤습니다. 많은 지적 부탁드립니다!