Skip to content

🚀 3단계 - GitHub(UI 상태) - #33

Open
simuelunbo wants to merge 11 commits into
next-step:simuelunbofrom
simuelunbo:step3
Open

🚀 3단계 - GitHub(UI 상태)#33
simuelunbo wants to merge 11 commits into
next-step:simuelunbofrom
simuelunbo:step3

Conversation

@simuelunbo

Copy link
Copy Markdown

안녕하세요
늦었지만 이번 3단계 미션도 리뷰 잘 부탁드립니다 🙏

@namjackson namjackson left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 준보님!
3단계 고생하셨습니다 :) 잘 구현해주셨네요 !
몇가지 참고하실만한 코멘트 드렸으니, 확인해주세요 :)
마지막까지 화이팅입니다!

import nextstep.github.ui.theme.GithubTheme

@Composable
fun GithubRepoSuccess(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GithubRepoSuccess라는 이름은 데이터 관점의 네이밍아닐까요?
GithubRepoList 같은 직관적인 네이밍이 좀더 가독성있지 않을까요 :)

@Composable
private fun GithubRepoSuccessPreview() {
GithubTheme {
GithubRepoSuccess(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리스트가 비어있을때는 어떤 화면이 나오나요?
요구사항 목록이 빈 경우에는 빈 화면 UI를 노출한다.(피그마 참조)을 추가해주세요 :)

message = context.getString(R.string.error_message),
actionLabel = context.getString(R.string.error_retry)
)
if (snackBarResult == SnackbarResult.ActionPerformed) run { viewModel::retryGithubRepo }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보통 범위 지정함수는 언제쓰시나요?
범위를 지정하거나, 가독성을 위해 사용하곤하지만
현재는 무의미 해보이고, 오히려 실수를 유발하는 위험한 코드는 아닐까요 :)

run { viewModel::retryGithubRepo } 의 결과값은 무엇일까요? :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 코드가 동작하는지 테스트할수있는 코드를 짜봐도 좋을거 같아요 :)

Comment on lines +105 to +114
val coroutineScope = rememberCoroutineScope()
val snackBarHostState = remember { SnackbarHostState() }
LaunchedEffect(Unit) {
coroutineScope.launch {
snackBarHostState.showSnackbar(
message = "에러 메세지",
actionLabel = "재시도"
)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

준보님이 생각하시는 Preview의 기준은 무엇인가요 :)
위의 로직은 Preview로 보긴 힘들지 않을까요?

이벤트들은 ui 테스트로 작성해보면 어떨까요? :)


private fun fetchRepositories() {
viewModelScope.launch(CoroutineExceptionHandler { _, throwable ->
_errorFlow.tryEmit(throwable)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러가 발생한다면, tryEmit이 잘 작동할까요? :)

}) {
_githubUiState.update { it.copy(isLoading = true) }
val repositories = githubRepoRepository.getRepositories("next-step")
_githubUiState.update { it.copy(repositories = repositories, isLoading = false) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

준보님이 생각하는 UiState를 설계하는 기준이 무엇인가요?
순수하게 궁금합니다 :)

Comment on lines +39 to +40
LaunchedEffect(true) {
viewModel.errorFlow.collect {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

강의 시간에서도 다룬 내용이긴한데,
https://medium.com/androiddevelopers/viewmodel-one-off-event-antipatterns-16a1da869b95
준보님의 의견이 궁금합니다! (의견만 주셔도 좋을거 같아요!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants