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

Feature/#795 게시글 화면과 댓글 화면 ux 개선 #807

Merged

Conversation

ki960213
Copy link
Collaborator

@ki960213 ki960213 commented Oct 30, 2023

#️⃣ 연관된 이슈

close : #795

📝 작업 내용

소프트 키보드 관련 함수를 공통적인 확장 함수로 선언했습니다.

showKeyboard()hideKeyboard()를 공통적인 확장 함수로 선언했습니다.

각종 커스텀 뷰를 만들었습니다.

  • BasicTextInputWindow (댓글 작성 입력창, 채팅 입력창)
  • SubtextInputWindow (댓글 수정 입력창)
  • NetworkErrorView (네트워크 에러 시)

리사이클러 뷰를 사용할 때 공통적으로 사용될 수 있는 ItemDecoration을 common 패키지로 옮겼습니다.

  • CommonRecyclerViewDivider (리사이클러 뷰 아이템 사이에(마지막은 제외) 앱 디자인 상 공통적으로 사용되는 장식(색상은 연한 회색에 높이는 0.5dp)을 쉽게 추가할 수 있도록 하는 클래스, 아마 검색 결과 목록에 사용하면 좋을 것 같습니다.)
  • IntervalDecoration (리사이클러 뷰 아이템 간격을 쉽게 추가할 수 있도록 하는 클래스)

댓글 관련 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.AdapterDataObserveronItemRangeInserted를 구현한 객체를 어댑터에 등록하면 리사이클러 뷰의 아이템이 갱신된 후에 onItemRangeInserted가 호출되기 때문에 delay()를 사용하지 않아도 됩니다.

디자이너님은 하이라이팅할 댓글이 중간에 오시길 바라셨지만 어렵고 임팩트가 크지 않고, 수많은 댓글 중 어떤 댓글을 통해 들어왔는지 알 수 있다는 목표를 달성했기 때문에 일단은 맨 위로 위치하도록 했습니다. 이후에 시간이 된다면 고치겠습니다.

스크린샷 (선택)

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

예상 소요 시간 : 1일
실제 소요 시간 : 3일

💬 리뷰어 요구사항 (선택)

ChildCommentActivity를 구현하면서 생각나는 추가하고 싶은 컨벤션을 개인적으로 정리하고 적용했습니다.
FeedDetailActivity는 적용하지 않았고 새로운 코드 또한 의미 없으니 일단 넘어가주세요.

…mmonRecyclerViewDivider로 변경 및 불필요한 파일 삭제

- CommonRecyclerViewDivider는 마지막 항목의 아래에 구분선을 넣지 않고 높이가 0.5dp라는 특징이 있음
… 변환 로직을 실행하는 함수를 가진 BaseViewModel 구현

- 로딩 상태, 네트워크 에러 상태, 성공 상태로 변하게 하는 로직을 override 해야 함
- 알 수 없는 예외 발생 시 실행할 로직을 정의해야 함
- 댓글을 수정할 때 서브 입력창 커스텀 뷰를 사용하도록 변경
- 댓글 수정 모드로 변경 시 댓글의 마지막 문자열에 커서가 이동하도록 하는 기능 구현
…게시글 화면에서 이동했는지에 대한 정보를 인자로 받도록 수정
- comment_author_image -> comment_author_image_description
- 하이라이팅 된 댓글의 배경색이 약간 불투명한 색으로 칠해짐
- 클릭하면 하이라이팅이 해제됨
Copy link
Member

@tmdgh1592 tmdgh1592 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 8 to 9
private val width: Int = 0,
private val height: Int = 0,
Copy link
Member

Choose a reason for hiding this comment

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

저번 리뷰에 남겼던 내용인데 아직 반영이 안 되었네요.
Float 타입을 사용하는 것은 어떤가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 28 to 29
<variable
name="onClick"
Copy link
Member

Choose a reason for hiding this comment

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

무슨 말인지 잘 이해하지 못했습니다..
다시 한 번 설명해주세용

Copy link
Member

@tmdgh1592 tmdgh1592 left a comment

Choose a reason for hiding this comment

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

@ki960213
제 의견과 코드 개선에 대한 리뷰를 남겼습니다.
확인 후 코멘트 부탁드려요 🙌

@ki960213
Copy link
Collaborator Author

ki960213 commented Nov 2, 2023

서버에서 데이터를 불러오는 데 실패하거나 알 수 없는 에러 발생 시에도 성공 상태로 변하게 하나요?

네. 왜냐하면 뷰모델에서 설정하는 UI 상태는 비즈니스 로직과 상관없이 UI를 변경시키는 것이기 때문입니다. 우리는 성공 상태를 로딩 바가 보여지지 않고 네트워크 에러 화면이 보여지지 않는 상태를 FetchResult의 SUCCESS라고 정의했습니다. xml에서 데이터 바인딩을 사용해서 로딩 바와 네트워크 에러 화면의 visibility를 gone으로 설정하고 있죠. 만약 우리가 알 수 없는 에러 발생 시 알 수 없는 에러가 발생했다는 화면을 네트워크 에러 화면처럼 보여주는 것으로 디자인이 변경된다면 알 수 없는 에러 발생 시 성공 상태가 아니라 알 수 없는 에러 상태로 변경해야 합니다.

이 말은 지금은 에러가 발생해도 일단 SUCCESS로 구현할 거지만 나중에 알 수 없는 에러 화면을 보여줘야 하는 요구사항이 추가되면 그 때에는 에러 상태를 추가하겠다는 의미인가요?

사실 그냥 봤을 땐 실팬데 왜 성공으로 취급하지라는 생각밖에 들지 않습니다 😢

그냥 봤을 땐 실팬데(제가 말한 실패는 서버에서 요청에 대해 실패인 응답값을 받은 이벤트를 말합니다.)
왜 성공으로 취급하지(제가 말한 성공은 화면에 로딩 바와 네트워크 에러 화면을 보여주지 않는 UiState를 할당하는 것을 말합니다.)

확실히 이름이 FetchResult라서 요청 결과라고 오해할 수 있네요. 사실은 화면의 상태를 추상화한 것인데 말이죠.
그래서 화면의 상태를 나타내는 새로운 클래스를 만들었습니다.

enum class ScreenUiState {
    NONE, LOADING, NETWORK_ERROR
}

어떠신가요?

- app:layoutMarginTop -> app:layout_marginTop
- dp 단위와 픽셀 단위 둘 다 대응할 수 있도록 변경
…port 하여 가독성 증가

- 일반적으로 프로덕트 코드에서는 static import를 하지 않지만 Delegates는 해도 괜찮다고 생각
@ki960213 ki960213 force-pushed the Feature/#795-게시글_화면과_댓글_화면_UX_개선 branch from 27c07c0 to 93e995b Compare November 3, 2023 07:36
Copy link
Member

@tmdgh1592 tmdgh1592 left a comment

Choose a reason for hiding this comment

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

@ki960213
몇 가지 도움이 될만한 코멘트를 남겼습니다.
확인해주세요~

Comment on lines 20 to 22
abstract class BaseViewModel : ViewModel() {

protected val _screenUiState = NotNullMutableLiveData(ScreenUiState.NONE)
Copy link
Member

Choose a reason for hiding this comment

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

BaseViewModel은 해당 PR에서는 우선 제거하는 것이 좋아 보입니다.

Copy link
Collaborator Author

@ki960213 ki960213 Nov 3, 2023

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() {
Copy link
Member

Choose a reason for hiding this comment

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

해당 액티비티에도 Deprecated된 imm 메서드를 사용하고 있습니다.

Copy link
Collaborator Author

@ki960213 ki960213 Nov 3, 2023

Choose a reason for hiding this comment

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

FeadDetail과 ChildComment의 코드와 비슷하기 때문에 ChildComment의 코드부터 리뷰를 받고 리뷰에 맞게 반영하기 위해 리뷰어 요구사항에 FeadDetailActivity 관련 코드는 일단 넘어가달라는 요청을 했지만 반영하겠습니다. 😊

Comment on lines 9 to 11
fun EditText.showKeyboard() {
val imm = context.getSystemService(INPUT_METHOD_SERVICE) as InputMethodManager
postDelayed(
Copy link
Member

Choose a reason for hiding this comment

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

해당 코드를 사용해보시는 것을 추천드립니다.

Suggested change
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)
}

Copy link
Collaborator Author

@ki960213 ki960213 Nov 3, 2023

Choose a reason for hiding this comment

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

와우! currentFocus라는 게 있군요. 그리고 저희는 사실 포커스 된 뷰의 텍스트를 편집하기 위해 소프트 키보드를 보여주는 것이므로 제안하신 코드가 훨씬 좋은 것 같네요! 👍

Comment on lines 21 to 29
setOnRefreshListener {
val refreshJob = onRefreshListener.onRefresh()
if (!refreshJob.isCancelled) {
CoroutineScope(Dispatchers.Main).launch {
refreshJob.join()
isRefreshing = false
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

다음 코드를 제안해봅니다~

Suggested change
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
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

early return을 깜빡했군요 😢
MainScope()라는 새로운 api도 알아가서 좋네요 👍

@ki960213 ki960213 merged commit 7e9f85d into android-main Nov 8, 2023
1 check passed
@ki960213 ki960213 deleted the Feature/#795-게시글_화면과_댓글_화면_UX_개선 branch November 8, 2023 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 안드로이드 관련 이슈 Low Priority 리뷰 우선순위가 낮은 PR 기능 추가 새로운 기능 추가 및 기존 기능 변경
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants