-
Notifications
You must be signed in to change notification settings - Fork 2
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/#795 게시글 화면과 댓글 화면 ux 개선 #807
The head ref may contain hidden characters: "Feature/#795-\uAC8C\uC2DC\uAE00_\uD654\uBA74\uACFC_\uB313\uAE00_\uD654\uBA74_UX_\uAC1C\uC120"
Conversation
…mmonRecyclerViewDivider로 변경 및 불필요한 파일 삭제 - CommonRecyclerViewDivider는 마지막 항목의 아래에 구분선을 넣지 않고 높이가 0.5dp라는 특징이 있음
… 변환 로직을 실행하는 함수를 가진 BaseViewModel 구현 - 로딩 상태, 네트워크 에러 상태, 성공 상태로 변하게 하는 로직을 override 해야 함 - 알 수 없는 예외 발생 시 실행할 로직을 정의해야 함
- 댓글을 수정할 때 서브 입력창 커스텀 뷰를 사용하도록 변경 - 댓글 수정 모드로 변경 시 댓글의 마지막 문자열에 커서가 이동하도록 하는 기능 구현
…하면 제대로 맨 아래로 스크롤하도록 수정
…게시글 화면에서 이동했는지에 대한 정보를 인자로 받도록 수정
…게시글_화면과_댓글_화면_UX_개선
- comment_author_image -> comment_author_image_description
- 하이라이팅 된 댓글의 배경색이 약간 불투명한 색으로 칠해짐 - 클릭하면 하이라이팅이 해제됨
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.
제 의견과 코멘트를 추가했습니다!
확인 부탁드립니다~
android/2023-emmsale/app/src/main/java/com/emmsale/presentation/common/extension/EditTextExt.kt
Outdated
Show resolved
Hide resolved
private val width: Int = 0, | ||
private val height: Int = 0, |
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.
저번 리뷰에 남겼던 내용인데 아직 반영이 안 되었네요.
Float 타입을 사용하는 것은 어떤가요?
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.
...mmsale/app/src/main/java/com/emmsale/presentation/common/recyclerView/SpaceItemDecoration.kt
Outdated
Show resolved
Hide resolved
...oid/2023-emmsale/app/src/main/java/com/emmsale/presentation/common/views/NetworkErrorView.kt
Show resolved
Hide resolved
<variable | ||
name="onClick" |
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.
무슨 말인지 잘 이해하지 못했습니다..
다시 한 번 설명해주세용
.../2023-emmsale/app/src/main/java/com/emmsale/presentation/ui/feedDetail/FeedDetailActivity.kt
Outdated
Show resolved
Hide resolved
android/2023-emmsale/app/src/main/res/layout/layout_sub_text_input_window.xml
Show resolved
Hide resolved
...sale/app/src/main/java/com/emmsale/presentation/ui/childCommentList/ChildCommentViewModel.kt
Show resolved
Hide resolved
...oid/2023-emmsale/app/src/main/java/com/emmsale/presentation/common/views/NetworkErrorView.kt
Outdated
Show resolved
Hide resolved
...msale/app/src/main/java/com/emmsale/presentation/common/bindingadapter/ViewBindingAdapter.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.
@ki960213
제 의견과 코드 개선에 대한 리뷰를 남겼습니다.
확인 후 코멘트 부탁드려요 🙌
...msale/app/src/main/java/com/emmsale/presentation/common/bindingadapter/ViewBindingAdapter.kt
Outdated
Show resolved
Hide resolved
...msale/app/src/main/java/com/emmsale/presentation/common/bindingadapter/ViewBindingAdapter.kt
Show resolved
Hide resolved
...msale/app/src/main/java/com/emmsale/presentation/common/bindingadapter/ViewBindingAdapter.kt
Outdated
Show resolved
Hide resolved
...msale/app/src/main/java/com/emmsale/presentation/common/bindingadapter/ViewBindingAdapter.kt
Outdated
Show resolved
Hide resolved
- SingleLiveEvent 적용 - BaseUiState를 사용함으로써 CommentsUiState에서 화면의 상태를 유지하지 않도록 변경 - BaseUiEvent를 사용함으로써 ChildCommentsUiEvent에서 대부분의 화면에서 공통적으로 처리하는 UiEvent를 분리
그냥 봤을 땐 실팬데(제가 말한 실패는 서버에서 요청에 대해 실패인 응답값을 받은 이벤트를 말합니다.) 확실히 이름이
어떠신가요? |
- app:layoutMarginTop -> app:layout_marginTop
- dp 단위와 픽셀 단위 둘 다 대응할 수 있도록 변경
- onClick -> onCommentClick
…port 하여 가독성 증가 - 일반적으로 프로덕트 코드에서는 static import를 하지 않지만 Delegates는 해도 괜찮다고 생각
27c07c0
to
93e995b
Compare
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.
@ki960213
몇 가지 도움이 될만한 코멘트를 남겼습니다.
확인해주세요~
abstract class BaseViewModel : ViewModel() { | ||
|
||
protected val _screenUiState = NotNullMutableLiveData(ScreenUiState.NONE) |
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.
BaseViewModel은 해당 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.
BaseViewModel
을 적용한 코드를 보여드리기 위해 ChildCommentViewModel
만 적용하여 푸시했는데, 논의 후 추가할 지 말 지 결정하는 게 좋겠네용
@@ -28,19 +29,44 @@ import dagger.hilt.android.AndroidEntryPoint | |||
@AndroidEntryPoint | |||
class FeedDetailActivity : AppCompatActivity() { |
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.
해당 액티비티에도 Deprecated된 imm 메서드를 사용하고 있습니다.
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.
FeadDetail과 ChildComment의 코드와 비슷하기 때문에 ChildComment의 코드부터 리뷰를 받고 리뷰에 맞게 반영하기 위해 리뷰어 요구사항에 FeadDetailActivity
관련 코드는 일단 넘어가달라는 요청을 했지만 반영하겠습니다. 😊
fun EditText.showKeyboard() { | ||
val imm = context.getSystemService(INPUT_METHOD_SERVICE) as InputMethodManager | ||
postDelayed( |
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 EditText.showKeyboard() { | |
val imm = context.getSystemService(INPUT_METHOD_SERVICE) as InputMethodManager | |
postDelayed( | |
fun Activity.showKeyboard() { | |
val imm = getSystemService(AppCompatActivity.INPUT_METHOD_SERVICE) as InputMethodManager | |
currentFocus?.postDelayed({ | |
imm.showSoftInput(currentFocus, InputMethodManager.SHOW_IMPLICIT) | |
}, 100) | |
} |
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.
와우! currentFocus
라는 게 있군요. 그리고 저희는 사실 포커스 된 뷰의 텍스트를 편집하기 위해 소프트 키보드를 보여주는 것이므로 제안하신 코드가 훨씬 좋은 것 같네요! 👍
setOnRefreshListener { | ||
val refreshJob = onRefreshListener.onRefresh() | ||
if (!refreshJob.isCancelled) { | ||
CoroutineScope(Dispatchers.Main).launch { | ||
refreshJob.join() | ||
isRefreshing = false | ||
} | ||
} | ||
} |
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.
다음 코드를 제안해봅니다~
setOnRefreshListener { | |
val refreshJob = onRefreshListener.onRefresh() | |
if (!refreshJob.isCancelled) { | |
CoroutineScope(Dispatchers.Main).launch { | |
refreshJob.join() | |
isRefreshing = false | |
} | |
} | |
} | |
setOnRefreshListener { | |
val refreshJob = onRefreshListener.onRefresh() | |
if (!refreshJob.isCancelled) return@setOnRefreshListener | |
MainScope().launch { | |
refreshJob.join() | |
isRefreshing = false | |
} | |
} |
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.
early return을 깜빡했군요 😢
MainScope()
라는 새로운 api도 알아가서 좋네요 👍
…를 위해 소프트 키보드를 보여주도록 변경
…막으로 이동하도록 하는 기능 추가
…를 사용하여 새로 정의한 함수로 변경
…e() 함수를 사용하여 코드 리팩터링
…게시글_화면과_댓글_화면_UX_개선
… 클래스명 변경 - SpaceItemDecoration -> IntervalItemDecoration
#️⃣ 연관된 이슈
📝 작업 내용
소프트 키보드 관련 함수를 공통적인 확장 함수로 선언했습니다.
showKeyboard()
와hideKeyboard()
를 공통적인 확장 함수로 선언했습니다.각종 커스텀 뷰를 만들었습니다.
리사이클러 뷰를 사용할 때 공통적으로 사용될 수 있는 ItemDecoration을 common 패키지로 옮겼습니다.
댓글 관련 layout xml 파일을 하나로 합쳤습니다.
원래는 item_all_comment, item_all_child_comment, item_all_deleted_comment, item_all_deleted_child_comment를
Space
를 사용하여 item_all_comment로 합쳤습니다. 그리고 공통적인 조건에 의해visibility
가 결정되는 경우Group
을 사용하여 한 번에 설정할 수 있도록 했는데, 이게 뷰에 직접 설정하는 게 아니라서 "어, xml 코드를 보면 visibility를 설정하지 않았는데 왜 사라진거지? 아, Group에서 설정해놨구나" 하는 경우가 생길 수 있다고 생각하는데, 혹시Group
을 사용하여 한 번에 설정하는 것 어떻게 생각하시나요?SwipeRefreshLayout의 onRefresh 바인딩 어댑터 설정 시 로딩 상태를 나타내는 애니메이션이 새로고침이 끝나면 사라지도록 하는 기능 구현
이를 위해 우리는 ViewModel에
Job
을 반환하는 메서드를onRefresh
에 설정해주어야 합니다.현재는 하위 호환성을 위해
onRefresh1
이라는 속성명을 사용합니다.BaseViewModel을 만들었습니다.
BaseViewModel 논의
댓글 화면 UX 개선
구현 기능
delay()
를 사용하지 않고 어떻게 정확하게 스크롤 할 수 있는가?RecyclerView.AdapterDataObserver
의onItemRangeInserted
를 구현한 객체를 어댑터에 등록하면 리사이클러 뷰의 아이템이 갱신된 후에onItemRangeInserted
가 호출되기 때문에delay()
를 사용하지 않아도 됩니다.디자이너님은 하이라이팅할 댓글이 중간에 오시길 바라셨지만 어렵고 임팩트가 크지 않고, 수많은 댓글 중 어떤 댓글을 통해 들어왔는지 알 수 있다는 목표를 달성했기 때문에 일단은 맨 위로 위치하도록 했습니다. 이후에 시간이 된다면 고치겠습니다.
스크린샷 (선택)
예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)
예상 소요 시간 : 1일
실제 소요 시간 : 3일
💬 리뷰어 요구사항 (선택)
ChildCommentActivity
를 구현하면서 생각나는 추가하고 싶은 컨벤션을 개인적으로 정리하고 적용했습니다.FeedDetailActivity
는 적용하지 않았고 새로운 코드 또한 의미 없으니 일단 넘어가주세요.