-
Notifications
You must be signed in to change notification settings - Fork 1
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/YAF-000] 네비게이션 로직을 통합합니다. #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
네비게이션을 나누는데는 정답이 없다고 생각합니다.
동현님이 말씀하신대로 지금 navigator 모듈의 역할이 명확하지 않고, 약간 core:common이랑 중복된 책임이 발생하고 있는 것 같긴 합니다!
그래서 app이나 common으로 옮기는 것도 좋은 방법 인 것 같습니다.
개발 하다가 좋은 레퍼런스나 생각이 나면 그때 그때 마이그레이션 하는 방법으로 갑시다이~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3
여기 구조가 반복되는거 같아서
OnboardingDestination.entries.forEach { destination ->
composable(destination.route) {
val viewModel = it.sharedViewModel<OnboardingViewModel>(navigator.navController)
LaunchedEffect(viewModel) {
viewModel.container.sideEffectFlow.collect { sideEffect ->
navigator.handleSideEffect(sideEffect, viewModel, onFinishOnboarding)
}
}
when (destination) {
...
}
이런식으로 빼는건 어떻게 생각하시나요?LaunchedEffect랑 ViewModel 호출 최소화 할 수 있을 것 같아서
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fun NavGraphBuilder.onboardingNavGraph(
navigator: OrbitNavigator,
onFinishOnboarding: () -> Unit,
) {
navigation(
route = OnboardingDestination.Route.route,
startDestination = OnboardingDestination.Explain.route,
) {
OnboardingDestination.routes.forEach { destination ->
composable(destination.route) { backStackEntry ->
val viewModel = backStackEntry.sharedViewModel<OnboardingViewModel>(navigator.navController)
LaunchedEffect(viewModel) {
viewModel.container.sideEffectFlow.collect { sideEffect ->
handleSideEffect(sideEffect, navigator, viewModel, onFinishOnboarding)
}
}
when (destination) {
OnboardingDestination.Route, OnboardingDestination.Explain -> {
OnboardingExplainRoute(viewModel)
}
OnboardingDestination.AlarmTimeSelection -> {
OnboardingAlarmTimeSelectionRoute(viewModel)
}
OnboardingDestination.Birthday -> {
OnboardingBirthdayRoute(viewModel)
}
OnboardingDestination.TimeOfBirth -> {
OnboardingTimeOfBirthRoute(viewModel)
}
OnboardingDestination.Name -> {
OnboardingNameRoute(viewModel)
}
OnboardingDestination.Gender -> {
OnboardingGenderRoute(viewModel)
}
OnboardingDestination.Access -> {
OnboardingAccessRoute(navigator, viewModel)
}
OnboardingDestination.Complete1 -> {
OnboardingCompleteRoute(viewModel)
}
OnboardingDestination.Complete2 -> {
OnboardingCompleteRoute2(viewModel, onFinishOnboarding)
}
}
}
}
}
}
enum class가 아닌 sealed class 라서 entries가 기본적으로 없어서... routes 사용해서 했습니당
|
||
@Composable | ||
inline fun <reified T : ViewModel> NavBackStackEntry.sharedViewModel(navController: NavController): T { | ||
val navGraphRoute = destination.parent?.route ?: return viewModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p4
단순궁금!
혹시 null 반환되면 어떤 동작이 일어나나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null 이면 return viewmodel()
이 호출되서 해당 composable에 해당하는 뷰모델이 만들어지쥬
Related issue 🛠
closed #40
어떤 변경사항이 있었나요?
CheckPoint ✅
PR이 다음 요구 사항을 충족하는지 확인하세요.
Work Description ✏️
1. 네비게이션 구조 개선
NavBackStackEntry
기반 확장 함수 추가.2. 네비게이션 공통화 작업
core:common
모듈로 이동:core:common
의navigation
패키지로 통합.3. 컴포넌트 네이밍 수정
MainScreen
→OrbitNavHost
MainNavigator
→OrbitNavigator
MainBottomNavigationBar
→OrbitBottomNavigationBar
MainNavTab
→TopLevelDestination
4. Home, MyPage 라우팅 구조 개선
HomeDestination
을 통해 Home 관련 라우팅 경로를 관리하도록 변경MyPageDestination
을 통해 MyPage 관련 라우팅 경로를 관리하도록 변경Uncompleted Tasks 😅
To Reviewers 📢
문제 상황
기존
onboarding
모듈에서는 하나의OnboardingViewModel
을 사용하고, 화면 이동에 필요한sideEffect
로직도 하나의ViewModel
에서 모두 정의되어 있었습니다.navigation
안에서 하나의OnboardingViewModel
을 생성하고 주입하려 했으나,navigation
은composable
이 아니기 때문에 ViewModel 생성이 불가능했습니다.현재 해결 방법
handleSideEffect
함수sideEffect
를 처리하기 위해 공통 함수로 분리:각 composable 내에서 ViewModel 생성 및 handleSideEffect 호출:
기존 다른 모듈은
Route
안에서 SideEffect 처리를 하고있어서 어떻게 바꿀지 논의가 필요할 것 같습니다.추가로
navigator
모듈 안의 네비게이션 로직의 상당 수가core:common
으로 이전되면서 해당 모듈의 역할이 거의 없어져서 만약 이 구조로 간다면 현재 내용을app
모듈로 옮기고navigator
모듈을 삭제하는 것도 좋을 것 같습니다.