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

[스터디 상세보기] 버그 리포팅 기반 개선 #251

Merged
merged 11 commits into from
Aug 9, 2023

Conversation

no1msh
Copy link
Collaborator

@no1msh no1msh commented Aug 8, 2023

😋 작업한 내용

  • 메인버튼 안내글씨가 너무 길어서 수정(온점도 삭제)
  • 스터디장이 신청자를 받으면 바로 멤버가 표시되어야하는데 반영 안되던 현상 수정
  • 개설한스터디 관리화면에서 스터디 시작 조건을 만족하지 못했는데 버튼이 활성화 되어있던 현상 수정
  • 뒤로가기 버튼 설명 추가

🙏 PR Point

빠뜨리거나 구동시에 제대로 확인 안되는게 있다면 언제든 DM 주세요.
테스트 용으로 스터디 신청 해달라는 요구도 DM 주세요 즉각 반응 합니다.

👍 관련 이슈

@no1msh no1msh added android💚 안드 feat 기능 추가 ßändäl🌗 반달 labels Aug 8, 2023
@no1msh no1msh self-assigned this Aug 8, 2023
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Test Results

13 tests   13 ✔️  3s ⏱️
  7 suites    0 💤
  7 files      0

Results for commit 264ea9b.

♻️ This comment has been updated with latest results.

@@ -16,12 +16,12 @@ sealed class StudyDetailState(
@DrawableRes
val subButtonSrc: Int,
) {
object Master : StudyDetailState(
data class Master(private val canStartStudy: Boolean) : StudyDetailState(
appBarTitle = R.string.study_detail_app_bar_study_master_title,
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 uiState는 뷰의 의존성을 갖게 되었습니다.
혹시 뷰모델에서 뷰를 알고있는 코드가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

뷰마들에서 뷰를 알고있는 코드는 현재 없습니다.

companion object {
private const val START_MEMBER_CONDITION = 2

fun canStartStudy(numberOfCurrentMembers: Int): Boolean =
Copy link
Collaborator

Choose a reason for hiding this comment

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

익명객체 안에 위치한 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

공동객체 안에 위치한 이유는 StudyDetail 객체를 만들지 않고 스터디 시작할 수 있는 조건을 확인하기 위해서입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

스터디 시작 조건을 판단하여 시작버튼의 Enabled를 조절해야 했기 때문에 라이브데이터 프로퍼티를 하나 가지고 있었습니다.
이 라이브데이터의 타입이 Boolean 이기 때문에 최신화 해주려는 목적으로 공동객체에 구현하였습니다.
_canStudyStart.value = StudyDetail.canStartStudy(studyParticipants.size)

Copy link
Collaborator

@RightHennessy RightHennessy 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 -61 to +62
<string name="study_detail_study_accept_waiting">스터디장의 수락을 기다리고있어요.</string>
<string name="study_detail_study_start_waiting">스터디 시작을 기다리고있어요.</string>
<string name="study_detail_study_accept_waiting">수락을 기다리고 있어요</string>
<string name="study_detail_study_start_waiting">시작을 기다리고 있어요</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

private val _canStudyStart: MutableLiveData<Boolean> = MutableLiveData()
val canStudyStart: LiveData<Boolean> get() = _canStudyStart
private val _studyMemberCount: MutableLiveData<Int> = MutableLiveData()
val studyMemberCount: LiveData<Int> get() = _studyMemberCount

fun fetchStudyDetail(studyId: Long) {
viewModelScope.launch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

runCatching {
    val studyDetail = studyDetailRepository.getStudyDetail(studyId).toUIModel()
    studyDetail
}

이 부분 특별한 이유가 있는게 아니시라면 한줄로 줄여도 될 것 같습니다 ㅎ_ㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞네요 수정하였습니다.
참고로 저런 경우에 studyDetail에 커서를 두고 ⌥ + ⌘ + N / Ctrl + Alt + N 누르면 바로 지역변수를 없애버릴 수 있습니다. ㅎㅎ;

@@ -111,6 +114,19 @@ class StudyDetailActivity :
startActivity(ProfileActivity.getIntent(this, memberId))
}

private fun observeParticipantsCount() {
studyDetailViewModel.studyMemberCount.observe(this) {
if (studyDetailViewModel.state.value is StudyDetailState.Master) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

방장과 스터디원이 같은 뷰를 쓰니까 이런 분기처리가 계속 필요하군요..
image

Comment on lines 146 to 151
private fun observeCanStartStudy() {
studyDetailViewModel.canStudyStart.observe(this) { cantStartStudy ->
binding.btnStudyDetailMainNotStudyMaster.isEnabled = cantStartStudy
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 이 버튼은 스터디장과 스터디원 모두가 사용하는 버튼이 아닌건가용..?
    아니라면 btnStudyDetailMainNotStudyMaster 이 단어가 다른 사람에게는 헷갈릴 수 있을 것 같습니다.
  2. viewModel 내의 canStartStudy가 public으로 되어있는데 직접 isEnable을 옵저빙하면서 해주는 이유는 추가적인 분기처리 때문일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 사용자에 따른 분기처리가 필요했기에 버튼 하나를 고유적인 id를 가져가게 할 수 없었습니다. (ex.btnStudyDetailStudyMasterStart)
    때문에 큰 버튼을 메인버튼, dm버튼 같이 작은 버튼을 서브 버튼으로 추상화하여 사용하기로 결정하였습니다.
    물론 이건.. 제가 바꾼다고 인지는 하고 있었는데 까먹었었네요! 찾아주셔서 감사합니다.

  2. 스터디 시작 조건을 만족했냐 아니냐로 버튼의 enabled가 갈리고, 스터디 장 혼자쓰는 버튼이 아니기에 dataBinding으로 enabled 값을 넣어주기가 애매하였습니다.

Copy link
Member

@inseonyun inseonyun left a comment

Choose a reason for hiding this comment

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

고생쓰

getString(
R.string.study_detail_button_start_study,
studyDetailViewModel.studyMemberCount.value,
studyDetailViewModel.study.value?.peopleCount,
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
Collaborator Author

Choose a reason for hiding this comment

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

선희의 NonNullLiveData 사용해서 개선해보았습니다.

@@ -41,7 +45,9 @@ class StudyDetailViewModel private constructor(
_study.value = it
_studyParticipants.value = it.studyMembers
_isFullMember.value = it.peopleCount == _studyParticipants.value!!.size
Copy link
Member

Choose a reason for hiding this comment

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

이 친구 왤케 화나있어요?
!!!!!!!

@@ -95,6 +101,8 @@ class StudyDetailViewModel private constructor(
studyParticipants.find { it.id == memberId } ?: StudyMemberUIModel.DUMMY
Copy link
Member

Choose a reason for hiding this comment

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

더미 데이터 사용은 이제 지양하죠~
사용하더라도 develop 브렌치 병합에는 더미가 안 들어가게 해주세용

@no1msh no1msh merged commit 3210f4c into develop Aug 9, 2023
2 checks passed
@no1msh no1msh deleted the AN/feature/242-studydetail-inflate branch August 9, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android💚 안드 feat 기능 추가 ßändäl🌗 반달
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[스터디 상세보기] 버그 리포팅 오류 해결
4 participants