Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[AN-REFACTOR] 상성 화면 리팩토링 #38
[AN-REFACTOR] 상성 화면 리팩토링 #38
Changes from 9 commits
9e70ce0
b14b67b
f709e28
f452fc9
6925354
3daeb50
f0cb674
26ab19f
9b59fb9
ee13a33
a3bd3d6
1a66244
99c2754
1789002
b607b46
a848212
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
repeatOnStarted
단위로 함수로 뺄 수도 있을 것 같아요.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.
해당 함수의 네이밍을 showTypeSelection 로 바꾸는건 어떨까요?
display, show.. 비슷한 네이밍인데 통일성을 맞춰주셨으면 좋겠어요 🙃
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 사용하는 것으로 바꾸셨군요!! 👍👍👍👍👍
아까 이야기한 것처럼
LifecycleExtension
사용하는 것으로 바꾸는 것 리마인드 코멘트 달아놓겠습니다~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.
동의합니다 ㅎ ㅎ
repeatOnStarted
로 바꿔주세용이유는 따로 공부해보는게 좋을 것 같습니다!
https://kotlinworld.com/228
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.
오프에서 얘기했던것처럼 startSelection 은 어떨까요?
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.
저는 개인적으로 이 함수는
selectType
이 더 좋아보이네요.뷰모델 코드를 볼 때 헷갈렸어요.
apply
와selection
둘 다 너무 일반적인 네이밍 같아서요.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.
동의합니다 :D
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.
컨벤션 좋습니다 👍
인터페이스의 이름의 경우에는 Listener,Handler에 따라 추후 변경될 수도 있겠네요 !
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
는model
패키지에 들어있는 게 아닌가요?이 부분은 제가 헷갈리네요
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.
이것은 컨벤션으로 정해두면 좋을 듯 합니다!!
NowInAndroid 에서는 UiState 가 그리 크지 않으면 ViewModel 파일에 두는 것을 볼 수 있습니다.
https://github.com/android/nowinandroid/blob/main/feature/interests/src/main/kotlin/com/google/samples/apps/nowinandroid/feature/interests/InterestsViewModel.kt
조금 길면 따로 파일로 분리하는 것 같습니다.
https://github.com/android/nowinandroid/blob/main/feature/search/src/main/kotlin/com/google/samples/apps/nowinandroid/feature/search/SearchResultUiState.kt
저는 개인적으로 ViewModel 아래에 두는 것을 선호해요
ViewModel 로직 작성하기 편하기 때문이에요 ㅎ ㅎ