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

Feat : 학사 일정 데이트 피커 #263

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

huiwoo-jo
Copy link
Contributor

@huiwoo-jo huiwoo-jo commented Sep 4, 2024

📮 관련 이슈

✍️ 구현 내용

  • 학사 일정 데이트 피커 바텀 시트를 제작하였습니다.
  • 현재 일 기준 전년도와 다음년도 2월까지 학사 일정을 확인 할 수 있습니다.
  • 데이트 피커로 일정 설정 혹은 화살표로 일정 목록 변경이 가능합니다.
  • 일정 날짜 변동에 따라 데이트 피커에 반영되도록 제작하였습니다.

📷 구현 영상

date-picker.mp4

✔️ 확인 사항

  • 컨벤션에 맞는 PR 타이틀
  • 관련 이슈 연결
  • PR 관련 정보 연결 (작업자, 라벨, 마일스톤 등)
  • Github Action 통과

@huiwoo-jo huiwoo-jo added 🤗 FEATURE Develop this project 🩵희우 labels Sep 4, 2024
@huiwoo-jo huiwoo-jo requested a review from m6z1 September 4, 2024 03:28
@huiwoo-jo huiwoo-jo self-assigned this Sep 4, 2024
Copy link
Collaborator

@m6z1 m6z1 left a comment

Choose a reason for hiding this comment

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

코멘트 확인 부탁드려요! 로직을 한 번 더 생각해보시면 좋을 것 같습니다! MVVM을 사용하면 충분히 구현할 수 있을 것 같아용

class DatePickerDialog(
val year: Int,
val month: Int,
private val dateSelectedListener: ScheduleFragment,
Copy link
Collaborator

Choose a reason for hiding this comment

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

다이얼로그를 생성할 때 생성자로 fragment를 꼭 받아야 할까요?? 프래그먼트를 생성자로 받지 않고 다른 방법으로 구현해보시면 좋을 것 같아요 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이전에 Dialog를 사용한 부분에서 가져온거라 생각을 못해봤네요 수정해보겠습니다

Comment on lines 16 to 19
interface OnDateSelectedListener {
fun onDateSelected(year: Int, month: Int)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

인터페이스를 분리하면 좋을 것 같아요 👍

Comment on lines +29 to +33
private var _year = LocalDate.now().year.toString()
private var year = _year

private var _month = LocalDate.now().month.toString()
private var month = _month
Copy link
Collaborator

Choose a reason for hiding this comment

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

둘 다 접근제한자가 private인데 프래그먼트에서 배킹프로퍼티를 사용하는 이유가 있나요 ?

Copy link
Contributor Author

@huiwoo-jo huiwoo-jo Sep 20, 2024

Choose a reason for hiding this comment

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

기존 코드들을 참고하여서 배킹 프로퍼티의 의미를 제대로 생각 못한거 같습니다🤔
코드를 다시 살펴본 후 명확히 답변 드리곘습니다.

private var _binding: DialogDatepickerBinding? = null
private val binding get() = _binding!!

private val viewModel: ScheduleViewModel by viewModel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

학사일정 프래그먼트와 뷰모델을 공유하려면 activityViewModels를 쓰면 될 것 같아요! 따로 다이얼로그에서 뷰모델 인스턴스를 새로 생성하는 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

코드를 살펴보니 별도의 인스턴스를 생성할 이유가 없네요..
익숙한 코드를 사용하여 생각해보지 못한 부분이라 말씀하신 대로 activityViewModels로 수정하겠습니다.

month = month.toInt(),
dateSelectedListener = this
)
dialog.show(requireActivity().supportFragmentManager, "CustomDialog")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tag 값은 상수화해서 사용하는 게 좋을 것 같아요! 그리고 커스텀 다이얼로그 보다는 다른 네이밍을 사용하면 좋을 것 같습니당

Copy link
Collaborator

Choose a reason for hiding this comment

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

requireActivityrequireContext 의 차이가 무엇일까요! 어디에는 위에서 컬러값을 가져올 땐 requireContext를 사용하셨는데 요기선 requireActivity 를 사용하신 이유가 궁금해용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dialog Manager에서만 사용하는 부분으로 중요하다고 생각을 못하여 네이밍을 크게 신경쓰지 못했네요.
ScheduleDialog정도로 수정 후 Tag 상수화를 사용하겠습니다.

requireActivity 액티비티 반환으로 activity!! 와 동일한 작용으로 알고있습니다.
dialogactivity 상속 개념으로 인하여 사용한 부분입니다.

requireContext는 컬러 리소스값에 접근해야하기에 Context를 사용합니다.
getContext와 달리 null 값이 아닌 context를 필수적으로 전달한다고 하여 사용했습니다.
requireContext는 null 처리가 되지 않아 코드 확인 후 getContext 사용과 requireContext 중 뭐가 더 나을지 고민해보겠습니다.


private val viewModel: ScheduleViewModel by viewModel()

override fun onCreateView(
Copy link
Collaborator

Choose a reason for hiding this comment

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

fragment의 생명주기에 대해서 알고 계신가요 ? onCreateView에서 해야할 일과 onViewCreated에서 해야할 일을 분리하면 좋을 것 같슴니다
참고할 블로그

Copy link
Contributor Author

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 86
// 현재 연도
val currentYear = LocalDate.now().year

// 순환 막기
binding.numberpickerDialogDatepickerYear.wrapSelectorWheel = false
binding.numberpickerDialogDatepickerMonth.wrapSelectorWheel = false

// editText 설정 막기
binding.numberpickerDialogDatepickerYear.descendantFocusability =
NumberPicker.FOCUS_BLOCK_DESCENDANTS
binding.numberpickerDialogDatepickerMonth.descendantFocusability =
NumberPicker.FOCUS_BLOCK_DESCENDANTS

// 연도 최소/최대값 설정 및 출력 방식 설정
with(binding.numberpickerDialogDatepickerYear) {
minValue = currentYear - 1
maxValue = currentYear + 1
displayedValues=((minValue..maxValue).map{"$it 년"}.toTypedArray())
}

// 월 최소/최대값 설정 및 출력 방식 설정
with(binding.numberpickerDialogDatepickerMonth) {
minValue = 1
maxValue = if (year == currentYear + 1) 2 else 12

// 연도 선택에 따른 월의 최대값 동적 설정
binding.numberpickerDialogDatepickerYear.setOnValueChangedListener { _, _, newYear ->
maxValue = if (newYear == currentYear + 1) 2 else 12
}

displayedValues=((minValue..maxValue).map{"$it 월"}.toTypedArray())
}

// 초기 설정
binding.numberpickerDialogDatepickerYear.value = year
binding.numberpickerDialogDatepickerMonth.value = month

binding.tvDialogPermissionComplete.setOnClickListener {
viewModel.setDatePicker(
binding.numberpickerDialogDatepickerYear.value,
binding.numberpickerDialogDatepickerMonth.value
)
dateSelectedListener.onDateSelected(
binding.numberpickerDialogDatepickerYear.value,
binding.numberpickerDialogDatepickerMonth.value
)
dismiss()
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

메서드를 분리하면 가독성이 더 좋아질 것 같아욥

Comment on lines 80 to 83
dateSelectedListener.onDateSelected(
binding.numberpickerDialogDatepickerYear.value,
binding.numberpickerDialogDatepickerMonth.value
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

MVVM 패턴을 사용하면 이 로직은 없어도 될 것 같아요. 현재 상태는 다이얼로그에서 선택한 값을 fragment로 직접 넘기는 형태이고 뷰모델을 사용하지 않는 것 같습니다! 뷰모델을 사용해서 dialog 의 생성자 중 fragment 를 제거하고, fragment에서도 interface 를 제거하고, 뷰모델에서 observe 받은 값으로만 수정해도 될 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dialog 구현에 앞서 MVVM 형태의 로직을 고려를 제대로 못하고 익숙한 방식으로 진행한거 같습니다.
로직을 수정해보겠습니다.

Copy link
Collaborator

@m6z1 m6z1 left a comment

Choose a reason for hiding this comment

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

MVVM 로직 구조에 대해서 다시 답변드렸습니다!
코멘트 확인 부탁드려용

Comment on lines +32 to +33
_binding = DialogDatepickerBinding.inflate(inflater, container, false)
val view = binding.root
Copy link
Collaborator

Choose a reason for hiding this comment

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

뷰바인딩을 초기화하고 반환 이후에 binding 객체를 사용하는 게 더 안전해보입니다!

Comment on lines +72 to +75
// 초기 설정
binding.numberpickerDialogDatepickerYear.value = year
binding.numberpickerDialogDatepickerMonth.value = month

Copy link
Collaborator

Choose a reason for hiding this comment

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

초기값을 설정하는 건 onCreateView에서의 역할이 아닌 것 같습니다! 뷰바인딩이 세팅이 끝난 후인 onViewCreated로 옮겨보시죵

Comment on lines +18 to +20
private val year: Int,
private val month: Int,
private val listener: ScheduleClickListener
Copy link
Collaborator

Choose a reason for hiding this comment

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

생성자로 연도, 월, 클릭리스너를 받아야 할까요?

Comment on lines +83 to +90
binding.tvDialogPermissionComplete.setOnClickListener {
val year = binding.numberpickerDialogDatepickerYear.value
val month = binding.numberpickerDialogDatepickerMonth.value
val date = CalendarDay.from(year, month, 1)

listener.buttonClick(date = date)

dismiss()
Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 방법은 MVVM 패턴이 아닌 것 같습니다! view 변경 시 ViewModel 내 메서드 호출 > fragment 에서 viewModel의 파라미터 observing > 학사일정 변경
이 로직대로 가야 할 것 같아요! 지금 로직은 interface로 직접 값을 넘기는 것 같습니다.


override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {

return BottomSheetDialog(requireContext(), R.style.CustomBottomSheetDialogTheme)
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 친구는 onCreateView에서 작동을 안 하나요 ?
onCreateView에

setStyle(STYLE_NORMAL, R.style.WebsosoDialogTheme)

요 코드 넣어보시면 될 거예요

Comment on lines +5 to +7
interface ScheduleClickListener {
fun buttonClick(date: CalendarDay)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

인터페이스를 제거해보시죠!

Comment on lines +31 to +33
override fun buttonClick(date: CalendarDay) {
viewModel.setPickedDate(date)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 인터페이스를 만들지 않고, 다이얼로그에서 버튼을 클릭했을 때 viewModel.setPickedDate 를 사용하면 될 것 같아요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤗 FEATURE Develop this project 🩵희우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

학사 일정 데이트 피커 설정
2 participants