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] 페이지 이동 로직 개선 #379

Open
wants to merge 11 commits into
base: an/develop
Choose a base branch
from

Conversation

JoYehyun99
Copy link

작업 영상

Screen_recording_20241020_193233.mp4

작업한 내용

바이옴 페이지 ➡️ 배틀 페이지 상대 포켓몬 선택

  • 토글로 이동 모드 설정
  • 이동 모드 선택값 datastore에 저장
  • tooltip 사용하여 안내 문구 표시

PR 포인트

  • 심지 PR 머지 되면 포켓몬 상세 -> 배틀 페이지 연결 후 마무리할께요!
  • 배틀 페이지 -> 포켓몬 상세 이동은 [1.1.1/AN-FEAT] 배틀 페이지 스피드 및 타입 표시 #356 할때 추가될 것 같아요!
  • datastore 에 저장하기위한 localDatasource 네이밍을 LocalNavigationDataSource로 설정해놨는데 (뭔가 Biome이라고 하기에는 애매한것 같아서,,,) 그냥 localbiomeDatasource 이라고 둘까요???

🚀Next Feature

Copy link

@sh1mj1 sh1mj1 left a comment

Choose a reason for hiding this comment

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

API 이름이나 설계 측면에서 리뷰 남겼습니다.

ktlint 가 뭔가 잚못 되어 있는지, 들여쓰기만 바뀐 코드가 많네요..? 왜 이러지

현재 예니 코드에서 홈 화면에서 포켓몬 도감으로 이동하면 터져요!

원인:

->: 의존성 방향이라고 하면,
PokemonListActivity -> PokemonListViewModel
PokemonListViewModel -> DefaultDexRepository
DefaultDexRepository -> DefaultBiomeRepository
DefaultBiomeRepository -> LocalNavigationDataSource
LocalNavigationDataSource -> NavigationModeDataStore

그런데 koin 에서는 LocalNavigationDataSourceNavigationModeDataStore 에 대한 정보를 모름.

그래서 DataSourceModule 에
singleOf(::LocalNavigationDataSource)
DataStoreModule 에
singleOf(::NavigationModeDataStore 추가해주어야 합니다.

Unable to start activity ComponentInfo{poke.rogue.helper.dev/poke.rogue.helper.presentation.dex.PokemonListActivity}: org.koin.core.error.InstanceCreationException: 
Could not create instance for '[Factory: 'poke.rogue.helper.presentation.dex.PokemonListViewModel']'
Could not create instance for '[Singleton: 'poke.rogue.helper.data.repository.DefaultDexRepository',binds:poke.rogue.helper.data.repository.DexRepository]'
Could not create instance for '[Singleton: 'poke.rogue.helper.data.repository.DefaultBiomeRepository',binds:poke.rogue.helper.data.repository.BiomeRepository]'
No definition found for type 'poke.rogue.helper.data.datasource.LocalNavigationDataSource'. Check your Modules configuration and add missing type and/or qualifier!

@@ -63,6 +64,7 @@ firebaseCrashlyticsBuildtools = "3.0.2"
agp = { module = "com.android.tools.build:gradle", version.ref = "agp" }

# kotlin
balloon = { module = "com.github.skydoves:balloon", version.ref = "balloon" }
Copy link

Choose a reason for hiding this comment

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

skydoves 님의 balloon 라이브러리를 추가하셨군요
새로운 기술이나 외부 라이브러리 도입하면 6기-기술검토 채널 에서 검토 받아야 하지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

앗,,,
오직 개발 시간 단축 을 위한 외부 라이브러리이기 때문에 거기까진 생각 못한 것 같아요 🥹

viewModelScope + errorHandler,
SharingStarted.WhileSubscribed(5000),
BattleResultUiState.Idle,
)
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.

그러게요,,,? 다시 확인 해볼께용

@@ -98,6 +98,8 @@
<item>야생</item>
<item>다음 바이옴</item>
</string-array>
<string name="biome_navigation_mode_setting">배틀 모드 설정</string>
<string name="biome_navigation_mode_info">"포켓몬 클릭 시 기본적으로 상세 페이지로 이동합니다.\n 배틀 페이지로 가려면 배틀 모드를 설정해주세요!"</string>
Copy link

Choose a reason for hiding this comment

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

이거 설명 잘 썼다 💯

import kotlinx.coroutines.flow.Flow
import poke.rogue.helper.local.datastore.NavigationModeDataStore

class LocalNavigationDataSource(
Copy link

Choose a reason for hiding this comment

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

Battle 에서 어떤 곳으로 이동할 지에 대한 정보네요.
이게 꼭 Battle 에서 다른 곳으로만 이동하는 것인지에 대한 데이터가 아니라, 전체적인 Navigation 에 대한 데이터 소스니까 괜찮은 것 같아요!

이거 말고 딱히 다른 네이밍이 생각나지도 않네요

@@ -79,17 +79,24 @@ class DefaultBattleRepository(
it.id == skillId
} ?: error("아이디에 해당하는 스킬이 존재하지 않습니다. id: $skillId")

override suspend fun pokemon(pokemonId: String): Pokemon = pokemonRepository.pokemon(pokemonId)
Copy link

Choose a reason for hiding this comment

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

이 함수는 여기서 정의할 필요가 없다고 느껴집니다.
이 함수는 BatltleViewModel 의 165 라인에서만 사용하고 있는데,
그렇다면 BattleViewModel 에서 PokemonRepository 를 가지도록 해서, 호출하는 게 더 낫지 않을까요?

Copy link
Author

@JoYehyun99 JoYehyun99 Oct 20, 2024

Choose a reason for hiding this comment

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

해당 부분에 대해서 저도 고민이 되었는데요!
현재 BattleRepository에서도 PokemonRepository 를 가지고 있는 상황에서
BatltleViewModel 또한 PokemonRepository를 가지고 있는게 맞을까..? 라는 생각이 들더라구요!

그럼에도 뷰모델에서 PokemonRepository를 넣어주는게 맞을까요?_?
진짜 모르겠👀

@@ -79,17 +79,24 @@ class DefaultBattleRepository(
it.id == skillId
} ?: error("아이디에 해당하는 스킬이 존재하지 않습니다. id: $skillId")

override suspend fun pokemon(pokemonId: String): Pokemon = pokemonRepository.pokemon(pokemonId)

override suspend fun pokemonWithRandomSkill(pokemonId: String): PokemonWithSkill {
Copy link

Choose a reason for hiding this comment

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

pokemonWithRandomSkill 이라는 네이밍으로 지은 이유는 가장 처음에 있는 스킬을 선택하기로 되어있기 때문인가요?

그렇다면, 정말 random 이 아니여서 pokemonWithFirstSkill 이 더 나은 것 같아요

Copy link
Author

@JoYehyun99 JoYehyun99 Oct 20, 2024

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

뷰가 이렇게 보입니당

@JoYehyun99
Copy link
Author

아 그리고 저 토글 버튼 체크 하는 부분에도 single click 설정 해주어야 하나요?????? 😲

@JoYehyun99
Copy link
Author

JoYehyun99 commented Oct 20, 2024

그래서 DataSourceModule 에
singleOf(::LocalNavigationDataSource)
DataStoreModule 에
singleOf(::NavigationModeDataStore 추가해주어야 합니다.

@sh1mj1
엇 이미 되어있었는데,,,?????? 그래도 터지나요???
아니면 제가 추가 커밋하는데 시간이 겹친건가,,,,? 😲😲😲😲

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.

2 participants