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

✨ Comment layout and api binding #251

Merged
merged 8 commits into from
Aug 8, 2024
Merged

✨ Comment layout and api binding #251

merged 8 commits into from
Aug 8, 2024

Conversation

Hogu59
Copy link

@Hogu59 Hogu59 commented Aug 8, 2024

  • Comment layout implemented
  • Comment api binding implemented
    • ⭕ fetch, post implemented
    • ❌ delete is not yet implemented

@Hogu59 Hogu59 added ✨ feature new feature AN Android 💄 UI labels Aug 8, 2024
@Hogu59 Hogu59 requested review from ii2001 and kmkim2689 August 8, 2024 12:16
@Hogu59 Hogu59 self-assigned this Aug 8, 2024
Copy link
Contributor

@kmkim2689 kmkim2689 left a comment

Choose a reason for hiding this comment

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

악어 댓글 구현 정말 수고 많으셨습니다!
댓글 작성 및 조회가 구현되고 좋아하는 모습을 보니 저도 기분이 좋았습니다.
몇 가지 피드백 남기니 참고해주시면 감사하겠습니다.
마지막까지 화이팅입니다!

Comment on lines +38 to +47
private fun CommentResponse.toComment(): Comment =
Comment(
commentId = commentId,
userId = userId,
userImage = userImage,
userName = userName,
createdAt = createdAt,
message = message,
mine = mine,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

mapper 파일을 따로 두는 것은 어떠신가요...!

Copy link
Author

Choose a reason for hiding this comment

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

mapper로 분리하는게 맞겠네요. 빠르게 머지를 먼저 진행했다보니 다른 이슈에서 옮기도록 하겠습니다.

Comment on lines +42 to +52
/*val content = commentContent.value

viewModelScope.launch {
if (content != null) {
commentRepository.postComment(
recipeId = recipeId,
message = content,
)
}
}
*/
Copy link
Contributor

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.

에... 저런게 있었군요.... 삭제하겠습니다.
제출하는 코드에 주석을 남기다니.. 제가 급했던것 같습니다.

Comment on lines +26 to +33
/*CommentViewModelFactory(
recipeId = 1L,
commentRepository =
FakeCommentRepository(
dataSource = FakeCommentDataSource(),
),
DefaultSessionLocalDataSource(requireContext().dataStore),
)*/
Copy link
Contributor

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.

변명의 여지없이 죄-송 합니다! 삭제하겠습니다!

Comment on lines +33 to +35
private val _isCommentEmpty: MutableLiveData<Event<Boolean>> = MutableLiveData()
val isCommentEmpty: LiveData<Event<Boolean>>
get() = _isCommentEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

비어있는지의 여부는 이벤트가 아닌 상태(State)라고 생각합니다.
해당 변수가 댓글이 비워졌다가 아닌 비어"있다"라는 것임을 보았을 때, State로 처리하는 것이 더 적절할 것 같습니다!

Suggested change
private val _isCommentEmpty: MutableLiveData<Event<Boolean>> = MutableLiveData()
val isCommentEmpty: LiveData<Event<Boolean>>
get() = _isCommentEmpty
private val _isCommentEmpty: MutableLiveData<Boolean> = MutableLiveData(true) // 기본값을 true로 설정하여, 초기에는 비워져있음을 나타냄
val isCommentEmpty: LiveData<Boolean>
get() = _isCommentEmpty

Copy link
Author

Choose a reason for hiding this comment

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

해당 내용으로 댓글 작성란이 비워져 있는데 댓글 작성을 누르면 이벤트로 토스트를 띄우려 했습니다.
comment라는 네이밍을 많이 쓰는 곳이다보니 댓글이 없는 상태로 오인될 여지가 있네요.
isCommentContentEmpty 정도로 바꾸면 어떨까 싶은데, 괜찮다면 변수 이름을 변경해보는 거로 진행해봐도 될까요?

Comment on lines +74 to +76
if (commentContent.value.isNullOrEmpty()) {
return
}
Copy link
Contributor

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.

그러네요. 얼리리턴만 진행하려 했는데 이벤트로 무언가 띄워주는게 바람직하겠네요!

Comment on lines +77 to +81
val result =
commentRepository.postComment(
recipeId = recipeId,
message = commentContent.value!!,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

!! 연산자를 사용하기 보다는 다음과 같이 변수를 활용하는 것을 제안드리고 싶습니다!

Suggested change
val result =
commentRepository.postComment(
recipeId = recipeId,
message = commentContent.value!!,
)
val commentText = commentContent.value
if (commentText.isNullOrEmpty()) {
return
}
val result =
commentRepository.postComment(
recipeId = recipeId,
message = commentText,
)

Copy link
Author

Choose a reason for hiding this comment

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

새로운 변수에 입히면 null 에 대한 얼리리턴이 가능하겠군요! 알려주셔서 감사합니다!

Comment on lines +83 to +85
if (result.isSuccess) {
initializeComments()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

isSuccess로 판단하는 것도 좋은데, Result 객체의 onSuccess 콜백을 활용하는 것도 좋을 것 같습니다. 개인적으로는 Result 객체를 변수에 할당할 필요도 없고 이것이 가독성이 더 좋은 것 같아서요!

Suggested change
if (result.isSuccess) {
initializeComments()
}
commentRepository.postComment(
recipeId = recipeId,
message = commentContent.value!!,
).onSuccess {
initializeComments()
}

Copy link
Author

Choose a reason for hiding this comment

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

제가 Result 사용에 미숙했네요. 알려주셔서 감사합니다. 해당 코드는 수정하고, 혹시 제가 만든 코드가 더 있다면 단기간 내에 반영해두겠습니다.

@kmkim2689 kmkim2689 merged commit 788b47d into an/dev Aug 8, 2024
2 checks passed
@kmkim2689 kmkim2689 deleted the an/feat/171 branch August 8, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AN Android ✨ feature new feature 💄 UI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants