Skip to content
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

Step4 - GitHub(인기 저장소) #32

Open
wants to merge 13 commits into
base: oyj7677
Choose a base branch
from
Open

Conversation

oyj7677
Copy link

@oyj7677 oyj7677 commented Sep 9, 2024

저장소 star 수와 hot 텍스트 출력을 구현하였습니다.

이전 리뷰에 말해주신 Error가 UiState에 포함되는가에 대해서는 아직 정리하지 못했습니다.
error와 UiState

1. MainActivity에 있는 상태값들 stateful한 MainScreen 컴포넌트로 이동.
1. RepositoryEntity, GithubRepositoryInfo에 stars프로퍼티 추가.
2. 관련 코드 수정
1. Step3 완료 체크
2. Step4 프로그래밍 요구사항 및 기능 요구사항 작성
1. 저장소 칼럼 헤더에 Star 개수 노출
2.  Star 개수 50개 이상일 시 HOT 문구 노출
1. 저장소 Star 개수 노출
2. 저장소 Star 개수 50개 이상 시 HOT 텍스트 노출
1. domain 패키지 생성
2. GithubViewModel 파리미터 변경 (저장소 -> 유즈케이스)
3. GetGithubRepoUseCase 구현
1. RepositoryEntity -> GithubRepositoryData (data 패키지)
2. GithubRepositoryInfo -> RepositoryEntity (domain 패키지)
1. core 패키지를 data패키지, di패키지로 분리
1. github.component -> gihub.list.component
Copy link

@BeokBeok BeokBeok left a comment

Choose a reason for hiding this comment

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

Github(인기 저장소) 미션 하시느라 고생하셨습니다!
고민해볼만한 의견들을 코맨트로 작성하였으니, 확인해보신 후 다시 리뷰 요청 부탁드립니다. 🙇

MainScreen(
uiState = uiState,
snackbarHostState = snackbarHostState,
onClickSnackBar = { viewModel.getRepositories("next-step") }
Copy link

Choose a reason for hiding this comment

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

onClickSnackBar에 재시도 시 수행해야할 로직을 넣어주셨네요. 👍
다만, 이름만 봤을 때 재시도 시 수행해야할 로직이라는 것을 인지할 수 있을까요? 🤔
이름만 보고서 어떤 동작을 수행할 것인지 인지할 수 있으면 좋을 것 같은데 영재님은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

사용자의 행위에 맞춰서 네이밍을 하는 것보다 동작에 초점을 맞춰서 네이밍을 만드는게 더 직관적인 것 같습니다.
onRetry로 변경하니 로직을 더 잘 나타내는 것 같습니다.
감사합니다.

Scaffold(
topBar = { MainTopBar() }
topBar = { MainTopBar() },
snackbarHost = { SnackbarHost(hostState = snackbarHostState) }
Copy link

Choose a reason for hiding this comment

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

scaffold를 활용해주셨네요 👍

Comment on lines 60 to 75
is GithubRepositoryUiState.Error -> {
// 에러 화면
LoadingProgress(
modifier = Modifier.padding(paddingValues),
snackBar = {
ErrorSnackbar(
errorMessage = stringResource(id = R.string.text_snackbar_network_error),
actionString = stringResource(id = R.string.text_snackbar_action_retry),
snackbarHostState = snackbarHostState,
modifier = Modifier.padding(paddingValues),
onClickAction = { onClickSnackBar() }
)
val errorMsg = stringResource(id = R.string.text_snackbar_network_error)
val actionLabel = stringResource(id = R.string.text_snackbar_action_retry)

LaunchedEffect(snackbarHostState) {
val snackbarResult = snackbarHostState.showSnackbar(
message = errorMsg,
actionLabel = actionLabel,
)

if (snackbarResult == SnackbarResult.ActionPerformed) {
onClickSnackBar()
}
)
}
}
Copy link

@BeokBeok BeokBeok Sep 9, 2024

Choose a reason for hiding this comment

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

이전 리뷰에 말해주신 Error가 UiState에 포함되는가에 대해서는 아직 정리하지 못했습니다.

아직 정리를 못하셨군요. 😅
제가 영재님께 아래와 같이 질문을 드렸었는데요.

에러 발생 시 스낵바가 아니라 토스트가 노출된다고 했을 때, 토스트가 노출되는 것을 State로 볼 수 있을까요? 🤔

제가 조금 더 질문을 드려볼게요!
본 미션의 디자인 가이드에는 4가지 종류의 화면이 있는데요.

  • 목록
  • 로딩중
  • 빈리스트
  • 오류 발생

여기서, 빈리스트와 오류 발생에 대한 화면은 정말 다르다고 볼 수 있을까요?
다이얼로그, 팝업, 스낵바, 토스트 등 1회성으로 노출되는 경우에도 UiState로 볼 수 있는걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

화면 상태를 Loading과 Ready로 축소시키고 Ready에 에러와 빈 항목의 여부를 판단하는 프로퍼티를 추가하여 구현하였습니다.
에러가 발생하여 스낵바가 노출되는게 하나의 상태라기 보다는 상태를 구성하는 항목중 하나라고 생각을 바꿨습니다.

modifier = Modifier
.padding(horizontal = 16.dp)
) {
if(repositoryInfo.stars > 50) {
Copy link

Choose a reason for hiding this comment

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

star의 개수가 50개 이상인 경우에 대한 로직을 잘 구현해주셨네요. 👍
하지만 이 로직이 View의 로직인 것일까요? 🤔

Copy link
Author

@oyj7677 oyj7677 Sep 11, 2024

Choose a reason for hiding this comment

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

View보다는 data 클래스에서 수행하는게 관리하기 편할 것 같습니다.
지금은 50으로 고정되어있어 isHot을 멤버변수로 처리하였습니다.

상황마다 isHot의 조건이 변경된다면 아래 함수로 변경하여 사용할 수도 있을 것 같습니다.

fun isHot(count: Int) : Boolean {
  return starts > count
}

1. 사용자 행동 -> 로직의 동작에 초점을 맞춤
2. onClickSnackBar -> onRetry
1. view에서 data로 로직 이동
2. 멤버변수 isHot을 사용하여 노출 여부 판단
1. delay를 추가하여 상태변경을 놓치는 경우를 방지함.
1. Loading과 Ready로 구분
2. Ready안에 항목 없음과 에러 여부를 판단하는 boolean값 추가
3. 관련 코드 수정
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