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

✨ 💄 Recipe StepMaking layout #162

Merged
merged 10 commits into from
Aug 6, 2024
Merged

✨ 💄 Recipe StepMaking layout #162

merged 10 commits into from
Aug 6, 2024

Conversation

Hogu59
Copy link

@Hogu59 Hogu59 commented Aug 4, 2024

  • essential logics implemented
  • test code will be implemented after 2-prior merge requests.
  • api connection will be implemented after api-docs implemented.
  • until then, this code will use fakes (repo, datasource)

@Hogu59 Hogu59 added ✨ feature new feature AN Android 💄 UI labels Aug 4, 2024
@Hogu59 Hogu59 requested review from ii2001 and kmkim2689 August 4, 2024 07:20
@Hogu59 Hogu59 self-assigned this Aug 5, 2024
@Hogu59 Hogu59 changed the title ✨ Recipe StepMaking layout ✨ 💄 Recipe StepMaking layout Aug 5, 2024
import net.pengcook.android.data.model.step.RecipeStepResponse
import retrofit2.Response

class FakeRecipeStepMakingDatasource : RecipeStepMakingDataSource {
Copy link

Choose a reason for hiding this comment

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

Fake보단 Local이 좋다는 소리를 많이 들었습니다! 어떻게 생각하시나요?

Copy link
Author

@Hogu59 Hogu59 Aug 5, 2024

Choose a reason for hiding this comment

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

일단 제가 검색해볼때는 잘 안나오네요.
아마 local이라는 패키지 내에서 fake를 사용하는 의미가 아니었나 싶긴 합니다. fake, stub, mock 등은 종류이기 때문에 해당 네이밍 사용이 필요할 것 같다는 생각이 드네요. 혹시 제가 참고할만한 링크가 있다면 알려주시면 감사하겠습니다.

@@ -1,9 +0,0 @@
package net.pengcook.android.data.model.feed.step

data class RecipeStepResponse(
Copy link

Choose a reason for hiding this comment

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

요것도 id들이 위에있는게 예쁠것같아요

Copy link
Author

Choose a reason for hiding this comment

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

해당 내용은 삭제된 내용으로 수정이 불가합니다. 아래의 내용으로 답변을 갈음하겠습니다.

@@ -0,0 +1,9 @@
package net.pengcook.android.data.model.step

data class RecipeStepResponse(
Copy link

Choose a reason for hiding this comment

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

위와 동일합니다.

Copy link
Author

@Hogu59 Hogu59 Aug 5, 2024

Choose a reason for hiding this comment

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

그렇네요. 아이디가 위로 올라오는게 가독성 상으로 좋을것 같고,
두개의 아이디가 있다보니, 더 우선되는 stepId를 가장 윗줄로 옮겼습니다!

@@ -0,0 +1,7 @@
package net.pengcook.android.presentation.core.listener

interface AppbarDoubleActionEventListener {
Copy link

Choose a reason for hiding this comment

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

더블 액션 뭔가 어색합니다..
더블 클릭이 낫지 않을까요?

Copy link
Author

@Hogu59 Hogu59 Aug 5, 2024

Choose a reason for hiding this comment

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

더블클릭은 한번에 클릭을 두번 누르는 "따닥" 클릭이 생각나는게 일반적이지 않을까요?
https://namu.wiki/w/%EB%8D%94%EB%B8%94%ED%81%B4%EB%A6%AD#toc

두개의 행위를 추상화한다고 생각했는데, 행위를 액션으로 표기하는게 이상할까요?
더블클릭은 이미 세상에 존재하는 단어로 다른의미를 가지고 있어 다른 참고 의견이 있다면 부탁드립니다.

binding.appbarEventListener = viewModel
}

private fun observeViewModel() {
Copy link

Choose a reason for hiding this comment

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

밑에 내용들도 따로 함수로 만들면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

수정완료입니다!

get() = _quitStepMakingState

init {
initStepData(stepNumber.value!!)
Copy link

Choose a reason for hiding this comment

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

!!필요한가요??

Copy link
Author

Choose a reason for hiding this comment

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

!! 필요하네요!
저도 모르고 사용했는데 Livedata.value 함수는 nullable type을 리턴해서 여기서 사용하려면 non-null 표기가 있어야 하네요.

Copy link
Author

Choose a reason for hiding this comment

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

분기처리에 대한 내용이었군요.
수정하겠습니다!

Copy link
Contributor

@kmkim2689 kmkim2689 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 56 to +57
initBinding()
observeViewModel()
Copy link
Author

Choose a reason for hiding this comment

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

여기가... onViewCreated 입니다...ㅋㅋㅋ

Comment on lines +31 to +40
StepMakingViewModelFactory(
recipeId = 1,
maximumStep = 15,
recipeStepMakingRepository =
FakeRecipeStepMakingRepository(
recipeStepMakingDataSource =
FakeRecipeStepMakingDatasource(),
),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

나중에 pull 하시면 AppModule 쪽에서 가져다쓰는 게 좋을 것 같습니다...!

Copy link
Author

Choose a reason for hiding this comment

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

pull 해서 수정하도록 하겠습니다!

@@ -39,5 +65,45 @@ class StepMakingFragment : Fragment() {
private fun initBinding() {
binding.lifecycleOwner = this
Copy link
Contributor

Choose a reason for hiding this comment

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

lifecycleOwner를 this 대신 viewLifecycleOwner로 할당하는 것이 좋아보입니다. 메모리 누수 위험이 있기 때문입니다.

Copy link
Author

Choose a reason for hiding this comment

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

호오 그렇군요! 알려주셔서 감사합니다. 혹시 해당내용을 참고할만한 링크 공유해주실 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

알려주셔서 감사합니다!

Comment on lines +104 to +105
findNavController().popBackStack()
findNavController().popBackStack()
Copy link
Contributor

Choose a reason for hiding this comment

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

popBackStack()을 두 번 호출하는 이유가 있는지 궁금합니다!
또한 뒤로가기를 위해 popBackStack()을 사용하는 것은 문제점이 존제하는데, 아래 영상을 참조해주시면 감사하겠습니다.
https://youtu.be/aV-Yai4zDc0?feature=shared

Copy link
Author

Choose a reason for hiding this comment

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

이런 문제가 있는걸 몰랐네요;;
저 부분에서 두번 호출하는 이유는 사용자가 처음에 있었던 화면으로 돌려줘야한다고 생각했기 때문입니다.
카테고리를 보다가 레시피 스텝 제작을 하고 완료를 했다면 레시피 제작 화면으로 돌아가야할 것 같다고 생각했는데, 그렇다고 어디서부터 왔는지에 대한 데이터를 계속 들고 있는것이 과연 바람직한가에 대한 생각도 드네요.
흠....
아니면 홈화면으로바로 이동하게 하는것으로 해보면 어떨까 싶은데 어떤 생각이신지 궁금합니다.

추가적으로 그렇다면 quitStepMakingState를 옵저브 하는 곳에서도 popBackStack을 제거한다면 어떻게 하면 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

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

일단 네비게이션으로 변경해두었는데 세부적인 로직이 매끄럽지 않은것 같습니다. 리팩토링하면서 변경하도록 하겠습니다.

Comment on lines +133 to +138

sealed interface StepMakingUiEvent {
data object EmptyIntroductionState : StepMakingUiEvent

data object UploadIngImageState : StepMakingUiEvent
}
Copy link
Contributor

Choose a reason for hiding this comment

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

파일 분리 가능하실까요?

Copy link
Author

Choose a reason for hiding this comment

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

호오.. 이거 놓쳤네요...! 알려주셔서 감사합니당!

Copy link
Author

Choose a reason for hiding this comment

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

호오.. 근데 안써도 될것 같아서 삭제했습니다!

Comment on lines 56 to +57
initBinding()
observeViewModel()
Copy link
Author

Choose a reason for hiding this comment

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

여기가... onViewCreated 입니다...ㅋㅋㅋ

@Hogu59 Hogu59 force-pushed the an/feat/121 branch 2 times, most recently from 857ee9e to 8ff2208 Compare August 6, 2024 02:23
- implement an appbar which has two action on left and right side
  (as original "item_appbar_with_action" only has one action)
- color of default appbar changed to white
- implement RecipeStepResponse class
- change variable type of RecipeStep class
- complete checking icon
- string resources
- BindingAdapters modified to present empty image view
@ii2001 ii2001 merged commit 065ed2d into an/dev Aug 6, 2024
2 checks passed
@ii2001 ii2001 deleted the an/feat/121 branch August 6, 2024 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AN Android ✨ feature new feature 💄 UI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants