-
Notifications
You must be signed in to change notification settings - Fork 3
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
[스터디 상세보기 뷰] 스터디 시작 시 보여질 요일 선택 뷰를 구현합니다. #481
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.
고생 많았슴다 뷰 예쁘네유~
|
||
private val textViewDays: Map<TextView, DayOfWeek> by lazy { initTextViewDays() } | ||
|
||
private var canMultiSelect: Boolean = 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.
canMultiSelect의 Default값은 false이고, 생성자로 받게 되는 attributeSet은 nullable 합니다.
view_day_of_week_selector.xml 의 코드를 슬쩍 봤는데 default value인 SingleSelect에 대한 Background drawable 설정을 안 해주더라구요.
attributeSet이 없을 경우 의도한대로 동작하지 않을 수도 있을 거 같아요
view_day_of_week_selector의 각각의 dayOfTextView의 background에 default background를 지정해주면 어떨까요?
그러면 이 값이 false일 때 setTypedArray()
에서 초기화 해줄 필요 없을 거 같아요!
제가 코드를 잘못 이해했을 수도 있으니
한 번 확인 부탁드립니다.
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.
초기화 값으로 사용한 값이 default로 사용될 수 있다는 값으로 확실하게 인지 했어야하는데 놓친부분이라 수정하였습니다!
감사합니다!
private fun setupDayClickListeners() { | ||
dayTextViews.values.forEach { textView -> | ||
textView.setOnClickListener { it -> | ||
val clickedDay: DayOfWeek = textViewDays[it] ?: throw NullPointerException() |
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.
단순 Exception 발생과 당연히 null이 발생할 일이 없다면 requireNotNull()
을 사용해도 좋을 거 같습니다
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.
수정하였습니다!
dayTextViews.values | ||
.filterNot { it == textView } |
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.
dayTextViews는 Map<DayOfWeek, TextView>
아닌가요?
filterNot()을 통해 textView가 아닌 것들을 찾는 이유가 뭔가요??
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.
해당 코드는 textView들을 순차적으로 돌면서 하나씩 클릭리스너를 붙이는 반복문안에 있는 내용입니다.
해당 구문은 싱글 선택 모드일 경우, 예를들어 월요일이 선택되어있을 때 화요일을 선택하면 월요일이 선택 해제되는 로직입니다!
fun setSelectableDays(selectableDays: List<DayOfWeek>) { | ||
dayTextViews.keys.forEach { | ||
if (selectableDays.contains(it)) { | ||
dayTextViews[it]?.isEnabled = true | ||
return@forEach | ||
} | ||
dayTextViews[it]?.isEnabled = 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.
혹시 이 함수는 어떤 역할을 수행하는 함수 인가요??
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.
해당 함수는 싱글 선택 모드일 때 선택할 수 있는 요일들을 리스트로 넘겨 지정하게해주는 로직입니다.
피그마에서 보여드렸듯 싱글 선택 모드는 선택 가능한 요일은 어두운 초록 동그라미, 선택 된 요일은 밝은 초록 동그라미인데요,
그 중 어두운 초록 동그라미가 무엇이 될지 선택하는 함수입니다.
val textView: TextView = dayTextViews[day] | ||
?: throw NullPointerException("$day 에 해당하는 TextView가 존재하지 않습니다.") |
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.
이것도 위에 남겨드렸던 리뷰와 비슷한 흐름인데요
Map 컬렉션의 Get이 Nullable 하여 생기는 문제인데 해당 로직이 null 이 발생할 일이 없을 거 같단 생각이 들어서 requireNotNull도 괜찮을 거 같은데 어떻게 생각하시나요?
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 getSelectedDays(): List<DayOfWeek> { | ||
val selectedDayTextViews: List<TextView> = dayTextViews.values.filter { it.isSelected } | ||
return selectedDayTextViews.map { textViewDays[it] ?: throw NullPointerException() } |
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 getSelectedDaysSize(): Int { | ||
return dayTextViews.values.filter { it.isSelected }.size |
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.
아래 코드로 변경해도 될 거 같은데 아닌가염? (아님 망고~)
return dayTextViews.values.filter { it.isSelected }.size | |
return dayTextViews.values.count { it.isSelected } |
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.
이건 ㄹㅇ 바보 오브 레전드네요 수정하였습니다 ㅋㅋㅋ
binding.tvStudyStartBottomSheetBtnStart.setTextColor( | ||
resources.getColor( | ||
R.color.white, | ||
null, | ||
), | ||
) |
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.
enable에 따른 텍스트 컬러 변경 xml이 존재하는 거로 아는데 따로 설정해주는 이유가 있나용?
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.
여윽시 개발천재!
val selectedDaysSize: Int = | ||
binding.dowsStudyStartBottomSheetDayOfWeekSelector.getSelectedDaysSize() | ||
|
||
if (selectedDaysSize > 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.
마법숫자
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.
저는 0은 0 자체로 주는 뜻이 있다고 생각합니다.
현재 로직에서도 0의 특성을 그대로 살려 로직을 작성하였습니다.
선택한 요일의 갯수가 0보다 큰 수라면 -> 버튼을 활성화 한다.
0 보다 크다 -> 자연수이다.
따라서 상수화를 하지 않았던 것인데요. 혹시 어떻게 생각하시나요?
|
||
<com.created.team201.presentation.common.customview.dayofselector.DayOfWeekSelector | ||
android:id="@+id/dows_study_start_bottom_sheet_day_of_week_selector" | ||
android:layout_width="match_parent" |
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.
제약을 걸어서 0dp로 하는게 좋을 거 같습니다
docs에서도 0dp를 권장한다고 합니다 :)
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.
지나가다가 슬쩍 ..
@@ -154,7 +155,11 @@ class StudyDetailActivity : | |||
} | |||
|
|||
private fun onMasterClickMainButton() { | |||
studyDetailViewModel.startStudy(studyId) | |||
val studyStartBottomSheetFragment = StudyStartBottomSheetFragment(studyId) |
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.
Fragment의 생성자에 인자를 넣으면 어떤문제가 발생할까요?
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.
Bundleㅇ ㅔ넣어서 전달한 것이 아니라 이렇게 전달한 이유가 궁금하네요 (궁금궁금)
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.
BottomSheetDialog
에만 집중을 해서 순간 결국 fragment였다는걸 망각 했군요 수정하였습니다!
private val dayTextViews: Map<DayOfWeek, TextView> by lazy { initDayTextViews() } | ||
|
||
private val textViewDays: Map<TextView, DayOfWeek> by lazy { initTextViewDays() } |
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.
똑같은 값을 초기화해주는 것 같은데 key로 찾기 value로 찾기로는 부족해서 두개를 따로 분리해주신걸까요?
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.
사실은 <key, key> 와 같은 형태의 자료구조가 필요했습니다.
map의 경우 value로는 key값을 찾지 못합니다.
day enum으로도 해당하는 textview를 찾을 수 있고, textView로도 해당하는 day enum을 찾을 수가 없었기에 지금과 같은 방법을 사용하였습니다.
if (canMultiSelect) { | ||
dayTextViews.values.forEach { | ||
it.background = ResourcesCompat.getDrawable( | ||
resources, | ||
R.drawable.bg_day_of_week_selector_multi_select, | ||
null, | ||
) | ||
} | ||
} else { | ||
dayTextViews.values.forEach { | ||
it.background = ResourcesCompat.getDrawable( | ||
resources, | ||
R.drawable.bg_day_of_week_selector_single_select, | ||
null, | ||
) | ||
} | ||
} |
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.
코드중복이 많은데, 함수로 분리해보는 것도 좋을 것 같아요.
drawable resource id만 넘기는 식으로요
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.
추가로 context.getDrawable()
을 사용해보시는건 어떤가용?
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.
함수분리하였습니다!
혹시 context.getDrawable()
를 장려하시는 이유가 무엇일까요?
} | ||
} | ||
|
||
typedArray.recycle() |
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.
메서드 이름이 헷갈릴 수 있는데
실생활로 비유하자면 typeArray라는 바가지에 물을 담아두었던걸 비우고 새로운 물을 받는 행위
-> 이 행위 자체를 바가지(typedArray)를 재활용한다 라고 생각하면 좋을 것 같습니다.
참고
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.
커뷰 너무 커엽네요
수고했습니다 ~
😋 작업한 내용
중간에 일곱개 선택하고 취소되는거 취소버튼 시현한겁니다 ㅎㅎ
🙏 PR Point
아령하세여어어어어허잇!
반달입니다.
요일 선택 커스텀뷰를 만들어 구현하였습니다.
본래 좀 더 커스텀 할 수 있는 기능을 만들어 라이브러리 배포를 하려했지만, 머리가 깨질거 같아 우리 프로젝트에나 잘돌아가는 뷰로 만들자고 해서 좀 쳐낸 기능이 많습니다.
그러다 보니 라이브러리로 만드려 했던 잔재가 남아있을 수 있습니다.
(ex: 시작 요일 커스텀 기능, 우리는 월요일로 시작하지만 일요일 시작으로도 할 수 있게끔)
현재는 inflate라 요일을 선택하고 시작하기를 눌렀을 때 선택한 요일의 Enum 상수값을 토스트로 출력하게끔 하였습니다.
서버 연결시 변경 예정입니다.
ps: 아 그리고 피그마 보여드렸듯 해당 요일 선택 커스텀뷰는 후에 피드에서도 사용될 예정입니다.
👍 관련 이슈
기획의 어떤 부분을 구현 / 수정했는가 (굉장히 상세하게 적어주세요, 해당 커밋의 링크, 코드의 위치를 남겨주면 더욱 좋습니다.)
스터디 시작 통신
을 하는게 아닌 요일을 선택할 수 있는 바텀 시트 다이얼로그가 표시됩니다.