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

✨ add taking time form for step making #358

Merged
merged 6 commits into from
Oct 7, 2024
Merged

✨ add taking time form for step making #358

merged 6 commits into from
Oct 7, 2024

Conversation

github-actions[bot]
Copy link

Original issue description
  • add step making time form view

closes #357

@github-actions github-actions bot added ✨ feature new feature AN Android labels Sep 12, 2024
Copy link

@ii2001 ii2001 left a comment

Choose a reason for hiding this comment

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

debuging을 위한 print만 없애주시면 완벽할 것 같습니다!

그런데 이렇게 request를 보내면 받을때 API수정은 필요 없겠죠?

Comment on lines 297 to 298
println("minute : $minute")
println("second : $second")
Copy link

Choose a reason for hiding this comment

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

요거 지워주시면 감사하겠습니다!

@Hogu59 Hogu59 self-requested a review October 7, 2024 00:32
Copy link

@Hogu59 Hogu59 left a comment

Choose a reason for hiding this comment

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

케이엠 안녕하세요!

코드 수정하시느라 수고 많았습니다!
아직 이해가 되지 않는 부분이 하나 있는데, 그 부분은 오늘 오프라인으로 설명 들으러 가겠습니다!

몇가지 자잘한 수정사항들이 있는데, 반영해주시거나 다른 의견이 있으시면 알려주시면 감사하겠습니다!

아울러, 저희 디자인 변경시 컨벤션을 수정해보면 좋을것 같아요!
pr에 이미지 캡쳐 추가가 되면 pr을 확인할 때 조금 더 용이할 것 같은데, 한번 같이 논의해보면 좋을 것 같습니다!

코드 수정하시느라 수고 많으셨고, 반영 후 알려주시면 감사하겠습니다!

@@ -70,5 +70,6 @@ class DefaultRecipeStepMakingRepository
description = description ?: "",
imageUri = imageUri ?: "",
image = imageTitle ?: "",
cookingTime = cookingTime ?: "00:00:00",
Copy link

Choose a reason for hiding this comment

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

null 처리 굿입니다!

@@ -73,6 +73,7 @@ fun RecipeStepEntity.toRecipeStepMaking(): RecipeStepMaking =
image = imageTitle ?: "",
imageUri = imageUri ?: "",
stepId = id,
cookingTime = cookingTime ?: "::",
Copy link

Choose a reason for hiding this comment

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

null 이면 "::"이렇게 표시하는게 의도된거죠..? 00:00:00 이런 방식이 아닌거죠..?

Copy link
Contributor

Choose a reason for hiding this comment

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

null일 경우는 사실상 이뤄질 일이 없긴 한데, 없으면 그냥 빈 값으로 edittext에 들어가도록 하려고 했습니다.
근데 일단 뷰모델에서도 시간이 비어있으면 00:00:00으로 처리하는 만큼 통일성이 있으면 좋기야 할 것 같습니다.

@@ -202,8 +202,8 @@ class RecipeMakingFragment : Fragment() {
val etHour = binding.itemTimeRequired.etTimeAmountPicker.etHour
val etMinute = binding.itemTimeRequired.etTimeAmountPicker.etMinute
val etSecond = binding.itemTimeRequired.etTimeAmountPicker.etSecond
etHour.filters = arrayOf(MinMaxInputFilter(0, 23))
Copy link

Choose a reason for hiding this comment

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

시간에 맞는 필터사용 굿 👍

val second = secondContent.value
val minute = minuteContent.value
Copy link

Choose a reason for hiding this comment

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

minute이 second 아래로 옮겨진거 같은데, 이유가 있을까요?
직관적으로 생각하면 hour-min-sec 순서로 놔두는게 좋을것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

넵넵 좋습니다!

@@ -249,7 +268,11 @@ class StepMakingViewModel
sequence = stepNumber,
).onSuccess { recipeStep ->
if (recipeStep == null) return@onSuccess
val minute = recipeStep.cookingTime.split(SEPARATOR_TIME).getOrNull(1) ?: ""
val second = recipeStep.cookingTime.split(SEPARATOR_TIME).getOrNull(2) ?: ""
Copy link

Choose a reason for hiding this comment

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

하디의 코드였다면, 어김없이 매직넘버로 슬라이딩 태클 걸었을지도...?ㅋㅋㅋ

Copy link

Choose a reason for hiding this comment

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

image

@@ -29,6 +29,8 @@ class StepMakingViewModel
) : ViewModel(),
StepMakingEventHandler,
AppbarDoubleActionEventListener {
private val stepTraversalStatus = MutableList(maximumStep) { false }
Copy link

Choose a reason for hiding this comment

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

이 내용은 정확히 로직이 이해가 되지 않네요... 오프라인으로 설명 한번 들으러 가겠습니다!

Copy link

Choose a reason for hiding this comment

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

설명 잘 들었습니다! approve 하겠습니다!

Copy link

@Hogu59 Hogu59 left a comment

Choose a reason for hiding this comment

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

설명도 잘 들었고 반영 확인했습니다!
수고 많았습니다!

@@ -29,6 +29,8 @@ class StepMakingViewModel
) : ViewModel(),
StepMakingEventHandler,
AppbarDoubleActionEventListener {
private val stepTraversalStatus = MutableList(maximumStep) { false }
Copy link

Choose a reason for hiding this comment

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

설명 잘 들었습니다! approve 하겠습니다!

@kmkim2689 kmkim2689 merged commit f5c70ff into an/dev Oct 7, 2024
2 checks passed
@kmkim2689 kmkim2689 deleted the an/feat/357 branch October 7, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AN Android ✨ feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants