-
Notifications
You must be signed in to change notification settings - Fork 0
일기 쓰기 페이지 Repository + ViewModel #447
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
Conversation
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.
일단 간단하게 봤어..!
기획을 모르니 내가 코멘트한게 정말 맞는건지 잘 모르곘군...
interface DiaryRepository { | ||
fun getDiaryWriteInit(): Flow<DiaryWrite> |
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.
이거 Flow 반환하는게 주현이 형이랑 이렇게 하기로 얘기가 된건가?
Flow 반환하는게 더 좋은건 맞는데
우리도 하려다가 잘 안됐어서 아직 스누티티는 모든 곳에서 그냥 도메인 모델 주고 있고
내가 이번에 바꾼 것도 Result<T>
주는거긴 한데 (#442 참고) 얘기가 된건가 싶어서
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.
내 기억에는
사용자의 action 하나로 A -> B -> C 동작이 다 일어나야 하면
그걸 다 Repository에서 내려줄수는 없으니 어쨌든 Repository는 단일 결과를 줘야하고
ViewModel에서 잘 combine해서 적당하게 잘 uiState로 만들어야 하는데
그러려면 좀 크게크게 고쳐야 해서 잘 안됐던 듯..?
화면 하나 딱 잡고 그 화면 내에서 일어나는 일 모두 정리해서 잘 계획 짠다음에 하면 될지두
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.
아 그리구 뭔가 사소해보이는 것들 조차도 다 flow로 줘야하나라는 고민도 있었던것 같음
아니라면 어떤건 flow가 오고 어떤건 model이 오고 이래서 좀 별로일지도
val todayState: List<Boolean>?, | ||
val questionsState: List<List<Boolean>>?, | ||
val moreText: String?, |
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.
요거 혹시 nullable 줄일 수 있는지 한번 보는 것도 좋을 듯?
별 이유는 없고 nullable이 되는순간 가져다 쓸 때마다 null 체크 해줘야 될거라
...main/java/com/wafflestudio/snutt2/views/logged_in/home/settings/diary/DiaryWriteViewModel.kt
Outdated
Show resolved
Hide resolved
...main/java/com/wafflestudio/snutt2/views/logged_in/home/settings/diary/DiaryWriteViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/views/logged_in/home/settings/diary/DiaryWritePage.kt
Outdated
Show resolved
Hide resolved
scope.launch { | ||
delay(100) | ||
scrollState.animateScrollTo(toScrollOffset.value) | ||
} |
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.
이거 이제 봤는데 여기는 어쩌다가 delay라는게 들어가게 된거지
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.
이거 나도 약간 이상하다고 생각은 했는데
isTodayCompleted가 true로 바뀜 -> 아래쪽 렌더링됨
이후에 스크롤을 해야 되는데
바로 scroll 해버리면 렌더링되기 이전에 호출돼서 잘 안 되더라구
그래서 그냥 delay 박아놨어
...c/main/java/com/wafflestudio/snutt2/views/logged_in/home/settings/diary/DiaryWriteUiState.kt
Show resolved
Hide resolved
진입점 매번 넣었다 지웠다 하지 말고 debug 에서만 보이게 두는거 어때? |
private suspend fun handleDiaryWriteError(error: DomainError) { | ||
val displayMessage = displayMessageResolver.getDisplayMessage(error) | ||
_diaryWriteUiEvent.emit(DiaryWriteUiEvent.ShowToast(displayMessage)) | ||
} |
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.
이거 global error도 생각 해줘야 함!
그 중에서 navigation 발생시켜야 하는 AuthError만 잘 처리해주면 됨
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.
근데 그럼 AuthError가 모든 페이지에 발생 가능하니까
모든 repository가 storage를 주입받고 clearToken을 가지고 있어야 되는 건가??
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.
아 이거 그 Test 보고 헷갈린듯 한데
UserRepository에 logout하는 함수 있어서 그것만 쓰면 될 걸?
#446 보면 됨
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.
왜냐면 Test에 있는건 내가 그 오류를 내려고 억지로 clearToken 해준거고
실제 상황은 이미 token이 잘못됐든 api key가 잘못됐든 token이 없든 잘못된 상황이니까
로그아웃만 시키면 된다고 생각하긴 했어
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.
오 뭐지 내가 잘못 얘기했네 TestViewModel은 잘못 짜놨구나
근데 너는 제대로 잘 바꿔놨네 ㅋㅋㅋㅋ
interface DiaryRepository { | ||
fun getDiaryWriteInit(): Result<DiaryWrite> | ||
|
||
suspend fun saveDiaryWrite(diaryWriteData: DiaryWrite): Result<DiaryWrite> |
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.
이건 잘 모르는 얘기일수도 있는데
함수 이름이 save라서 하나 살펴보자면 return 할게 없으면 그냥 Result<Unit>
주면 됨
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.
ㅇㅎㅋㅋㅋㅋ 뭐 넣을까 고민하다가 그대로 넣었는데 그렇네..
UIState 실제 필요한 것들로 갈아엎기
상태 끌어올려서 UI 최상단에서 관리하기
ViewModel + Repository 구현
Navigation은 미구현 상태로 쪼개서 PR 올려요