-
Notifications
You must be signed in to change notification settings - Fork 9
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
[feature/#725] poke ananymous #732
Conversation
AnonymousFriend Lottie
|
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.
일단 리뷰 한번 보시고....이후에 수정된거 또 리뷰 남길게요
object : Animator.AnimatorListener { | ||
override fun onAnimationStart(animation: Animator) {} | ||
|
||
override fun onAnimationEnd(animation: Animator) { |
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.
이거 확장함수 만들어서 View.doOnAnimationEnd { }
같은걸로 사용하면 좋을듯
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.
오 이건 생각 못해봤는데 좋을 것 같네요!! 반영하겠습니다 :)
Handler(Looper.getMainLooper()).postDelayed({ | ||
layoutAnonymousFriendOpen.visibility = View.GONE | ||
viewModel.setAnonymousFriend(null) | ||
}, 2000) |
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.
lifecycleScope.launch
...?
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.
음... 둘의 차이점을 찾아보니 lifecycleScope를 쓰는게 더 좋을 것 같습니다! 수정하도록 하겠습니다 :)
@@ -283,7 +331,29 @@ class FriendListSummaryActivity : AppCompatActivity() { | |||
is UiState.Success<PokeUser> -> { | |||
messageListBottomSheet?.dismiss() | |||
viewModel.getFriendListSummary() | |||
showPokeToast(getString(R.string.toast_poke_user_success)) | |||
|
|||
if ((it.data.pokeNum == 5 || it.data.pokeNum == 6) && it.data.isAnonymous) { // 익명 베스트 프랜드 + 힌트 |
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.
Magic Number 지양해주세요, 그리고 이 조건이 무슨 조건인지는 주석만으로는 파악하기 힘들 수 있으니 조건 자체를 property나 함수로 빼는걸 권장합니다.
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.
넵 알겠습니다. 함수로 빼서 진행하도록 하겠습니다!
setAnimation(R.raw.friendtobestfriend) | ||
}.playAnimation() | ||
} | ||
} else if ((it.data.pokeNum == 11 || it.data.pokeNum == 12) && it.data.isAnonymous) { // 익명 소울메이트 + 정체 공개 |
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.
이것도 마찬가지
} | ||
viewModel.pokeSimilarFriendUiState.onEach { | ||
when (it) { | ||
is UiState.Loading -> "Loading" |
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.
이 부분은 기존에 그렇게되어있어 똑같이 했었던 거라 기존 코드도 전부 수정했습니다!
Loading일때는 아무 액션도 하지 않는 것으로 기존 의도를 파악해 아래와 같이 수정했습니다
is UiState.Loading -> {}
} | ||
|
||
false -> showPokeToast(getString(R.string.toast_poke_user_success)) | ||
false -> { | ||
if ((it.data.pokeNum == 5 || it.data.pokeNum == 6) && it.data.isAnonymous) { // 익명 베스트 프랜드 + 힌트 |
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.
저 부분만 수정하고 바로 머지하시면 될 것 같습니다.
fun View.addAnimatorEndListener(action: () -> Unit) { | ||
object : Animator.AnimatorListener { | ||
override fun onAnimationStart(animation: Animator) {} | ||
|
||
override fun onAnimationEnd(animation: Animator) { | ||
action() | ||
} | ||
|
||
override fun onAnimationCancel(animation: Animator) {} | ||
|
||
override fun onAnimationRepeat(animation: Animator) {} | ||
} | ||
} |
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.
저 코드는 지운 코든데 깃허브에 기록이 남아있어 버렸군요...
LottieAnimationView로 바꾼 코드에서 수정했습니다 :)
inline fun LottieAnimationView.addOnAnimationEndListener(crossinline action: () -> Unit) {
this.addAnimatorListener(object : Animator.AnimatorListener {
override fun onAnimationStart(animation: Animator) {}
override fun onAnimationEnd(animation: Animator) {
action()
}
override fun onAnimationCancel(animation: Animator) {}
override fun onAnimationRepeat(animation: Animator) {}
})
}
What is this issue?
Video
쉬운 연출을 위해 페이지 이동시 바로 뜨도록 했습니다. 실제와 다른부분이 있습니다.
friendtobestfriend.mp4
soulmate.mp4
To Reviewer