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

[1.1.1/AN-FEAT] 포켓몬 상세에서 배틀 도우미로 가는 floating 버튼 추가 #378

Merged
merged 43 commits into from
Oct 23, 2024

Conversation

sh1mj1
Copy link
Contributor

@sh1mj1 sh1mj1 commented Oct 19, 2024

작업 영상

floating.butotn.webm

작업한 내용

  • 포켓몬 상세에서 배틀 도우미로 가는 floating 버튼 추가
  • 버튼은 expandable floating Button 으로, 내 포켓몬, 상대 포켓몬
  • viewModel 에 이벤트와 sharedFlow 객체와 navigateToBattle 함수 만들어 놓음
  • 포켓몬 상세 액티비티에 observeNavigateToBattleEvent 함수 만들어 놓았음. -> 예니는 여기서 startActivity 만 하면 될듯?

PR 포인트

  • 이전 Coordinatorlayout PR 이후에 머지되어야 합니다.
  • floating button 을 만들고, 눌렀을 때 리스트 뷰가 만들어지게 했습니다.

🚀Next Feature

  • 위 기능 테스트 코드 작성
  • koin 적용

@JoYehyun99
Copy link
Contributor

그 배틀 페이지 내 포켓몬 실루엣이용!! (피카츄랑 팬텀 있는!)

Copy link
Contributor

@murjune murjune left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 심지 ~ 😉

코멘트 몇 가지 남겼으니 봐주세용

@@ -36,3 +39,26 @@ private fun PokeFilterUiModel.toParams(): List<AnalyticsEvent.Param> {
AnalyticsEvent.Param(key = "type", value = it.name)
} + AnalyticsEvent.Param(key = "generation", value = selectedGeneration.name)
}

fun AnalyticsLogger.logPokemonDetailToBattle(event: PokemonDetailToBattleEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

굿!

}
}

class MyPokemon(description: Int, icon: Int) : BattlePopUpUiModel(description, icon)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class MyPokemon(description: Int, icon: Int) : BattlePopUpUiModel(description, icon)
class MyPokemonUiModel(description: Int, icon: Int) : BattlePopUpUiModel(description, icon)

UiModel 에는 UiModel postfix를 붙여줘야합니다!

네이밍 컨벤션

Comment on lines 78 to 101
private fun initFloatingActionButton() {
val listPopupWindow = ListPopupWindow(binding.root.context)

listPopupWindow.apply {
setBackgroundDrawable(
ContextCompat.getDrawable(
binding.root.context,
android.R.color.transparent,
),
)

setAdapter(battlePopupAdapter)
anchorView = binding.fabPokemonDetailBattle

width = ListPopupWindow.MATCH_PARENT
height = ListPopupWindow.WRAP_CONTENT

isModal = true
}

binding.fabPokemonDetailBattle.setOnClickListener {
listPopupWindow.show()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

오옹 fab xml로는 한번도 안써봤는데 이렇게 하는거군요! LGTM👏

setBackgroundDrawable(
ContextCompat.getDrawable(
binding.root.context,
android.R.color.transparent,
Copy link
Contributor

Choose a reason for hiding this comment

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

drawbleOf 유틸 함수를 써도 좋을듯??

Copy link
Contributor

Choose a reason for hiding this comment

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

transparent 를 적용하는게 더 이쁜지 없는게 더 이쁜지 팀원들 의견을 들어봐도 좋을 것 같아용

Copy link
Contributor

Choose a reason for hiding this comment

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

스크린샷 2024-10-21 오후 4 10 43

transparent 대신 white같은 다른 색상을 넣으면 이렇게 영역을 차지하게 됩니다 !

convertView: View?,
parent: ViewGroup,
): View {
if (convertView != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

올만에 보네요 convertView ㅋㅋㅋ

)

launch {
analyticsLogger().logPokemonDetailToBattle(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
analyticsLogger().logPokemonDetailToBattle(
logger.logPokemonDetailToBattle(
  1. 이미 존재하는 logger를 써주세요~
  2. launch 로 analytics 로직을 감싸는 것은 바람직하지 않습니다. 만약 아날릭틱스 로거에서 예외가 발생하면 navigateToBattle 는 예외를 던지게 됩니다.

관련해서 좋은 블로그가 있어 추천드려요!

https://velog.io/@murjune/kotlin-Coroutine-supervisorScope-vs-SupervisorJob-%EC%96%B4%EB%96%A4%EA%B1%B8-%EC%82%AC%EC%9A%A9%ED%95%98%EB%9D%BC%EB%8A%94%EA%B1%B0%EC%A7%80#4-%EC%8B%A4-%EC%82%AC%EB%A1%80-analyticsscope

Copy link
Contributor

@kkosang kkosang left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 심지 !! 👍

floating버튼이랑 list를 같이 사용하면 width 설정이 어렵네요..! 동적으로 width 계산하는 것보다 list를 사용 안하는 편이 좋을지두..?!

val battleRoleValue =
when (battlePopUp) {
is MyPokemon -> "MyPokemon"
is EnemyPokemon -> "EnemyPokemon"
Copy link
Contributor

Choose a reason for hiding this comment

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

리마인드) Enemy말고 Opponent로 통일하기로 했습니다 !

setBackgroundDrawable(
ContextCompat.getDrawable(
binding.root.context,
android.R.color.transparent,
Copy link
Contributor

Choose a reason for hiding this comment

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

스크린샷 2024-10-21 오후 4 10 43

transparent 대신 white같은 다른 색상을 넣으면 이렇게 영역을 차지하게 됩니다 !

<com.google.android.material.floatingactionbutton.FloatingActionButton
android:id="@+id/iv_pop_up"
imageRes="@{battlePopUp.icon}"
android:onClick="@{() -> battlePopUpHandler.navigateToBattle(battlePopUp)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

TextView는 onSingleClick을 사용하고 있는데 이 부분은 사용안하고 있는 이유가 있나요??

setAdapter(battlePopupAdapter)
anchorView = binding.fabPokemonDetailBattle

width = ListPopupWindow.MATCH_PARENT
Copy link
Contributor

Choose a reason for hiding this comment

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

width를 MATCH_PARENT로 했을 경우, 리스트 팝업의 영역이 전체가 되어 팝업을 띄워놓은 후 스크롤하게 되면 스크롤이 안되는 것 처럼 보일 수 있겠네요..!
width를 Adapter를 기준으로 동적으로 측정하여 지정해주어야 한다는데 어렵네요..

@sh1mj1 sh1mj1 force-pushed the an/feat/pokemon-detail-navigate-battle-button branch from 26fc82b to e190064 Compare October 22, 2024 06:29
@sh1mj1 sh1mj1 force-pushed the an/feat/pokemon-detail-navigate-battle-button branch from e190064 to 1d9fe1b Compare October 22, 2024 06:31
@sh1mj1
Copy link
Contributor Author

sh1mj1 commented Oct 22, 2024

/noti
드디어 다시 플로팅 액션 버튼

작업: 아래 기존 -> 현재 로 fix & refactor, 간단한 버튼 확장, 축소 애니메이션

default.webm

배틀 선택 실루엣으로 하면 이렇게 보이게 됨
image

기존

listpopUpWIndow 와 어댑터를 사용해서 구현함.
textview 와 FloatingActionButton 을 뷰홀더로 사용.

기존의 문제점

API 의 한계. 구현이 어려움..
기존에는 anchorView 를 사용해서 위치 조정에 문제가 있음.
FloatingActionButton 의 너비를 지정할 때 문제가 생김.
스크롤 포커스를 뺏어가는 문제점...

현재

정적인 버튼(FloatingExtendedActionButton) 여러 개로 구현함.
기존의 문제점들 해결함.

Copy link
Contributor

@murjune murjune left a comment

Choose a reason for hiding this comment

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

애니메이션을 적용해주셨네요 👏 역시 심지의 애니메이션인가요?
코멘트 확인해주세요 ㅎㅎ

import poke.rogue.helper.R

// TODO: use koin
class FloatingButtonHandler(
Copy link
Contributor

Choose a reason for hiding this comment

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

Koin 을 적용할 필요가 없습니다!
Context, View 모두 화면 회전 즉, 액티비티가 재생성될 경우 죽기 때문이죠

그리고, 해당 class보다는 함수로 나타내는 것이 좋다 생각합니다!
해당 class는 toggle 의 기능만 하고 있기 때문입니다.

그리고 isExpanded 상태는 구성변경에 유실됩니다 😢

@@ -34,12 +31,11 @@ class PokemonDetailActivity :

private lateinit var pokemonTypesAdapter: PokemonTypesAdapter
private lateinit var pokemonDetailPagerAdapter: PokemonDetailPagerAdapter
private lateinit var floatingButtonHandler: FloatingButtonHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 네이밍만 봤을 때, 해당 객체가 어떤일을 하는지 유추하기가 어렵습니다 😢

@sh1mj1
Copy link
Contributor Author

sh1mj1 commented Oct 23, 2024

/noti

진짜 다 반영함

작업한 내용

  • conflict 해결
  • 클래스 분리한 거 취소. 함수로 가지게 함
  • isExpanded 를 lifecycle 에 맞춰 save & restore --> 구성 변경에도 유실되지 않음.

@sh1mj1 sh1mj1 merged commit 7174c8d into an/develop Oct 23, 2024
2 checks passed
@sh1mj1 sh1mj1 deleted the an/feat/pokemon-detail-navigate-battle-button branch October 23, 2024 02:53
@murjune
Copy link
Contributor

murjune commented Oct 23, 2024

/noti 1.1.0 과 버전에 들어가서 리버트함!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AN_FEAT ✨ 안드 새로운 기능 v1.1.1 🏷️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants