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

Step3 - GitHub(UI 상태) #28

Merged
merged 8 commits into from
Sep 7, 2024
Merged

Step3 - GitHub(UI 상태) #28

merged 8 commits into from
Sep 7, 2024

Conversation

oyj7677
Copy link

@oyj7677 oyj7677 commented Sep 5, 2024

UiState를 잘 구현했는지 모르겠네요.
아키텍처가 조금 부족한 것 같습니다. 해당 부분 유심히 봐주시면 감사하겠습니다.

몇가지 질문 드립니다.

  1. 스낵바의 SnackbarHostState() 경우 상태 호이스팅이 필요한가요?
  2. 에러상태의 경우 로딩 상태에서 스낵바만 추가되는 것 같아 LoadingProgress에 snackBar: @composable () -> Unit = {} 파라미터를 추가했는데 구조적으로 괜찮은가요?

1. Step2 기능 요구 사항 체크
2. Step3 프로그래밍 요구사항 및 기능 요구사항 작성
1. 원형 로딩 UI구현
2. GithubRepositoryUiState 생성
3. 응답값 저장 프로퍼티 제거(_githubRepositoryInfoList)
4. GithubRepositoryUiState의 성공 클래스에 응답 데이터 담아서 사용
1. 빈 리스트 상태 추가
2. 응답은 성공했지만 데이터가 없을 경우
3. GithubRepositoryEmpty 컴포넌트 추가
1. 스낵바를 활용한 재시도 요청 로직 구현
2. 깃허브 데이터 요청 실패 핸들링 로직 구현
3. LoadingProgress 컴포넌트 파라미터로 snackBar 추가
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(UI 상태) 미션하시느라 고생하셨습니다. 👍
고민해볼만한 의견들을 코맨트로 작성하였으니, 확인해보시고 다시 리뷰 요청 부탁드립니다. 🙇

Comment on lines +38 to +40
SnackbarResult.ActionPerformed -> {
onClickAction()
}
Copy link

Choose a reason for hiding this comment

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

에러 발생 시 스낵바가 노출되고, 스낵바 옆에 있는 "재시도" 버튼을 눌렀을 경우 에러가 발생하면 스낵바가 노출되지 않네요. 🤔
"재시도" 버튼을 눌렀을 때 에러가 발생하면 스낵바가 노출되어 "재시도" 를 할 수 있어야 할 것 같은데 영재님은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

네트워크 요청을 보낼 때 UiState를 Loading으로 변경시키는 로직을 추가하여 UiState가 에러 -> 로딩 -> 에러 로 변경되며 스낵바가 다시 노출되도록 변경하였습니다.


is GithubRepositoryUiState.Error -> {
// 에러 화면
LoadingProgress(
Copy link

Choose a reason for hiding this comment

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

에러가 발생하게 되면 로딩바는 노출되지 않아야할 것 같은데 영재님은 어떻게 생각하시나요?

Copy link

Choose a reason for hiding this comment

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

에러상태의 경우 로딩 상태에서 스낵바만 추가되는 것 같아 LoadingProgress에 snackBar: https://github.com/composable () -> Unit = {} 파라미터를 추가했는데 구조적으로 괜찮은가요?

구조적으로 괜찮지 않다고 생각하지 않으시는 이유가 궁금하네요. 🤔
Compose에서 제공하는 Row, Column, Box 등 어떻게 구현이 되어 있는지 살펴보시면 좋을 것 같습니다.

Copy link
Author

@oyj7677 oyj7677 Sep 6, 2024

Choose a reason for hiding this comment

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

한 번 더 생각할 수 있도록 답변 남겨주셔서 감사합니다.

개인적인 기준으로 4주차 과제의 피그마 자료 중 'Repository 목록 - 오류 발생' 이미지에서 로딩과 스낵바가 동시에 노출된다고 판단하였습니다.

LoadingProgress에 스낵바만 추가하면 된다고 생각하여 재사용을 목적으로 snackBar: @composable () -> Unit = {} 파라미터 추가하여 사용했습니다.

구조에 정답은 없겠지만 재사용성을 목적으로 했을 때 적절한지 여쭤본 것입니다.
아직 컴포즈에 익숙하지 않다보니 새로운 시도를 해본 것이 적절한지 판단하기 어려워 질문 남겼습니다.

마지막으로, 답변주신 Row, Column, Box에 대한 구현은 어떤 의도였는지 알 수 있을까요?

Copy link

Choose a reason for hiding this comment

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

@oyj7677
@Composable () -> Unit 타입을 넣어도 되는지를 고민하시는 것 같아서 Row, Column, Box가 내부적으로 어떻게 구현이 되어 있는지를 살펴보면 좋을 것 같다고 의견을 드렸습니다.

말씀하신대로 구조에는 정답은 없습니다.
다만, LoadingProgress에 스낵바를 추가하는 것이 적절한지에 대한 고민을 조금 더 해보셨으면 좋겠습니다.
만약, 대부분의 화면에서 스낵바가 노출될 가능성이 있다면, 관련 Composable 함수에 구현하신 snackBar 파라미터를 전부 넣어야 할 것 같은데 이 부분에 대해서 영재님은 어떻게 생각하시나요?

Scaffold에서 SnackBar를 노출할 수 있도록 지원을 하는데 Scaffold 로 사용할 수는 없을지도 고민해보시면 좋을 것 같습니다.

Comment on lines 18 to 23
fun ErrorSnackbar(
errorMessage: String,
actionString: String,
modifier: Modifier = Modifier,
onClickAction: () -> Unit
) {
Copy link

Choose a reason for hiding this comment

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

스낵바의 SnackbarHostState() 경우 상태 호이스팅이 필요한가요?

질문주신 배경이 궁금합니다!
만약, 상태 호이스팅이 필요하지 않다고 생각하신다면, 스낵바와 관련된 테스트는 어떻게 하면 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

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

우선 스낵바 컴포넌트를 StateLess하게 상태호이스팅 진행하였습니다.

상태호이스팅을 진행한다면 MainActivity까지 올라갈 것이고 그것이 과하다고 생각하였습니다.

테스트의 경우 클릭 이벤트에 관해서만 하면 될것이라 생각하였습니다.

Copy link

Choose a reason for hiding this comment

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

@oyj7677
오 저는 단순히 여쭤보았는데 SnackBarHostState를 위로 올리셨군요. 😅
영재님이 생각하시는 상태 호이스팅이란 무엇인지 궁금하네요. 😅
MainActivity까지 올려야만 상태호이스팅이 되는 건가요? 🤔
ErrorSnackbar 내부에서 상태를 가지지 않는 구조라면 MainActivity까지 올리지 않아도 ErrorSnackbar는 stateless를 유지할 수 있을 것 같은데 영재님은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

MainActivity까지 올라가야만 상태호이스팅이라고 생각하지 않았습니다.
ErrorSnackbar 컴포넌트에 대해서 stateless를 유지하고자 상태호이스팅을 적용하고 그 상태가 MainScreen에 있는 것도 적절하지 않다고 생각하여 MainActivity까지 올렸던 것입니다.
다시 생각해보니 MainScreen을 오버로딩하여 상태를 가지고 있는 컴포넌트와 없는 컴포넌트로 나누어 처리하면 MainActivity까지 올리지 않아도 될 것 같습니다.


sealed class GithubRepositoryUiState {

data object Error : GithubRepositoryUiState()
Copy link

Choose a reason for hiding this comment

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

UiState에 Error를 정의해주셨네요.
어떤 이유로 UiState에 Error를 정의하셨나요? 🤔
에러와 관련된 요구사항은 아래에 해당하는데요.

오류가 발생한 경우 재시도 가능한 스낵바를 노출한다.

스낵바를 노출하는 것이 UiState로 볼 수 있을까요? 🤔
강의 자료 중 UI 상태를 노출하는 방법 예시를 살펴보시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

저는 UI상태를 네가지로 보았습니다. (로딩, 성공, 실패, 빈 목록)
그렇기에 Error도 하나의 상태라고 생각하여 구현하였습니다.

제가 생각한 것은 2가지 입니다.
첫째, Error가 빠진다면 Empty도 같이 빠지고 Success 데이터 클래스에 프러퍼티로 isError, errorMsg, isEmpty가 추가된다.
그에따라 GithubRepositoryList는 GithubRepositoryUiState를 파라미터로 받고 isError와 isEmpty로 분기하여 화면을 노출시킨다.
이렇게 변경하는 것이 어렵다고 생각한 점은 Success라는 클래스에 isError가 들어가는 적절한 것인가? 들어가야한다면 Success의 네이밍을 어떻게 변경해야하는가? 입니다.

둘째, sealed class를 사용하지 않고 isLoading, isError, errorMsg, isEmpty, List<깃허브 데이터> 를 하나의 data 클래스로 관리하여 사용하는 것입니다.
두번째 방법을 사용하지 않은 이유는 sealed class를 사용하여 분기점을 확실하게 보여주는 것이 좋다고 생각하였습니다.

리뷰어님은 Error를 UiState로 정의하기에는 어떤 점이 과하다고 생각하셨는지 궁금합니다.

Copy link

Choose a reason for hiding this comment

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

@oyj7677
Error가 빠진다고 해서 Empty가 빠져야하는 이유가 무엇일까요?
응답을 제대로 받았지만 만족하는 데이터가 없는 경우도 Empty로 볼 수 있을 것 같은데요.
이 경우도 Error로 봐야하는 것인지요? 🤔

아마 컴포즈가 익숙지 않으셔서 어렵게 생각하시는 느낌이 드는데요. 😅
가령, 에러 발생 시 스낵바가 아니라 토스트가 노출된다고 했을 때, 토스트가 노출되는 것을 State로 볼 수 있을까요? 🤔

1. 통신 요청 시 UiState를 Loading으로 변경
2. 스낵바를 통한 재시도 요청 실패 시 스낵바가 노출되지 않는 이슈 해결
1. val snackbarHostState = remember { SnackbarHostState() } 값을 상태 호이스팅
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(UI 상태) 미션 피드백 반영하시느라 고생하셨습니다. 👍
고민해볼만한 의견들을 코맨트로 작성하였으니 확인 부탁드립니다. 🙏
그럼 다음 미션도 화이팅입니다! 💪

@BeokBeok BeokBeok merged commit 68e18dc into next-step:oyj7677 Sep 7, 2024
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