-
Notifications
You must be signed in to change notification settings - Fork 1
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
[#149] 투표 완료 api 구현 #162
[#149] 투표 완료 api 구현 #162
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.
현재는 중간에 투표취소하면 처음부터 다시 모두 투표해야하는 로직인게 맞지?!
요 위에 있는 확인사항 말고는 특별히 코멘트를 달만한 부분이 보이지 않네... 일단 어푸할게!!
@JeonK1 마자마자 그때 처음부터 다시 투표를 하자고 했던거 같아..! |
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.
고생하셔씀다. 좋아요 👍
VoteResultUiParam( | ||
eventId = voteUiState.value.eventId, | ||
voteResultIdList = ImmutableList(voteUiState.value.photoList.map { it.id }), | ||
).toDomainParam(), |
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.
이거 UiParam이라는게 나는 좀 어색한데 바로 도메인 모델 쓰는건 별로인가?
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.
오.. 다시 생각해보게 되는거 같아..! 지금 드는 생각은 진짜 리퀘스트용 모델이라서 뭔가 ui단에서 조작해서 쓸일이 없을거 같고 , 리퀘스트 할때 필드 형식에 맞춰서 보내줘야하니깐 ui용 리퀘스트 모델이 필요없을거 같다고 생각이 들긴하네..? 맞는 생각일까...?
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 finishVote() { | ||
// TODO: api 스펙 정의되면 리스트를 서버에 보내는 로직 구현하기 | ||
viewModelScope.launch { | ||
// TODO: 나중에 예외 처리 방법 결정되면 onSuccess,onFailure 구현하기 |
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.
onSuccess에서 투표 완료화면 이동하는거는 이 PR에서 추가되도 좋을듯@?
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.
아아아 오키오키 화면 이동 작업해 놓을게~~~
data class VoteResultResponse( | ||
@Json(name = "event_id") | ||
val eventId: Long, | ||
) |
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.
하...........ㅂㄷㅂㄷ
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.
형 고생했어.. 일단 보이는대로 코멘트 남겨보았으니 확인 부탁해!
...ntation/src/main/java/com/mashup/gabbangzip/sharedalbum/presentation/ui/vote/VoteActivity.kt
Outdated
Show resolved
Hide resolved
...ntation/src/main/java/com/mashup/gabbangzip/sharedalbum/presentation/ui/vote/VoteActivity.kt
Show resolved
Hide resolved
...c/main/java/com/mashup/gabbangzip/sharedalbum/presentation/ui/vote/navigation/VoteNavHost.kt
Outdated
Show resolved
Hide resolved
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.
코멘트내용 해결된 것 완료!
어푸할게~
9b3d790
to
ab5fb79
Compare
Issue No
Overview (Required)
Screenshot