[Feature/#471] 북마크에서 세션 목록으로 리다이렉팅 기능 구현#480
Conversation
|
@winter-love-dev ci쪽 이슈 확인해주세요 |
Test Results53 tests 53 ✅ 24s ⏱️ Results for commit 16d579b. ♻️ This comment has been updated with latest results. |
|
CI 이슈 대응 마쳤습니다. 🙌 |
| data class SessionDetail(val sessionId: String) : Route | ||
|
|
||
| @Serializable | ||
| data class SessionListScrollTo(val sessionId: String) : Route |
There was a problem hiding this comment.
session - api 모듈에 보면 sessionList 데이터가 있어요. 여길 확인해주세요.
There was a problem hiding this comment.
Router에서 sessionId를 추가한 케이스를 만들면 두개의 세션 리스트가 없지 않을지
| return | ||
| } | ||
| viewModelScope.launch { | ||
| navigator.navigateBack() |
There was a problem hiding this comment.
Q) navigateBack을 하신 이유가있나요? 현재 화면이 남아있어도 될것 같은데
There was a problem hiding this comment.
백스택상 홈-세션 만 남기고 싶어서 그런 것 같은데 맞을까요?
There was a problem hiding this comment.
Q) navigateBack을 하신 이유가있나요? 현재 화면이 남아있어도 될것 같은데
기존에 세션 목록 화면으로 이동하는 흐름이 홈 탭 -> 세션 목록 이었기 때문에,
해당 피처 구상할 때 기존 흐름의 일관성을 유지하려는 목적이었습니다.
백스택상 홈-세션 만 남기고 싶어서 그런 것 같은데 맞을까요?
그래서 북마크에서 홈 탭으로 어떻게 이동할까 고민했는데, 딱 이 생각이 나서 navigateBack() 을 하자는 결론이 되었슴다.
There was a problem hiding this comment.
생각해보니
- 북마크 화면 -> 세션 목록으로 갔을 때
- 뒤로가기 -> 북마크 화면
이게 흐름상 더 나을것 같군요.
| modifier: Modifier = Modifier, | ||
| highlightSessionId: String? = null, | ||
| onSessionClick: (Session) -> Unit, | ||
| ) { |
There was a problem hiding this comment.
여기 부분 정렬 detekt 페일 날것 같아보이네요. 체크 해주세요
| ) | ||
| } | ||
|
|
||
| composable<SessionListScrollTo> { navBackStackEntry -> |
There was a problem hiding this comment.
라우트 세션과 세션 리스트 스크롤이면
세션 리스트가 결국 두개가 되는거 아닌지. 기존 세션에 기본값을 세션 아이디를 비워두면 같은거 아닐까요? 예를 들면 홈에서 현재 진행중인 세션으로 바로 이동해야 한다면 라우트가 하나더 추가되어야 할 이유는 없을것 같아요. 데이터 클래스를 사용 하면 간단히 해결이 가능할것 같은데
There was a problem hiding this comment.
넵 SessionScreen 을 띄우는 로직을 굳이 둘로 나눌 필요 없을것 같습니다. 말씀해주신 방향으로 좀 더 간단한 버전으로 개선해볼게요 🙌
|
두 가지 개선 사항 반영했습니다.
2025-06-03.2.00.54.mov |
| val highlightState by animateColorAsState( | ||
| targetValue = if (isHighlighted) { | ||
| KnightsColor.Blue03.copy(alpha = 180f) | ||
| } else { | ||
| MaterialTheme.colorScheme.surface | ||
| }, | ||
| animationSpec = tween( | ||
| durationMillis = 300, | ||
| easing = FastOutSlowInEasing, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
이건 상태가 아니라 그냥 색상이어서 tint나 color 정도로 설정해주시는게 좋을 것 같습니다.
| var highlightState = rememberHighlightState( | ||
| sessionState = sessionState, | ||
| scrollToSessionId = scrollToSessionId | ||
| ) |
| lazyColumnIndex++ | ||
| } | ||
| } | ||
| return -1 |
There was a problem hiding this comment.
이런 magic number도 상수화되면 좋을 것 같아요
| groups.forEachIndexed { groupIndex, group -> | ||
| group.sessions.forEachIndexed { sessionIndex, session -> | ||
| if (session.id == sessionId) { | ||
| return lazyColumnIndex | ||
| } | ||
| lazyColumnIndex++ | ||
| } | ||
| } |
There was a problem hiding this comment.
(사소) 추가로 이런건 Collections API로 로직을 더 간결하게 만들어볼 수 있을 것 같아요
| ) { | ||
| BookmarkContent( | ||
| uiState = bookmarkUiState, | ||
| onClickedRedirectItem = onSessionClick, |
There was a problem hiding this comment.
onSelectedItem = viewModel::selectSession,
부분을 참고하셔야 할것 같네요. 지금 방식은 새로운 경로를 추가하지 않을려고 걷어낸 부분을 다시 추가하신 상황이라
|
Navigation 객체를 viewModel에서 주입받고 이를 통해 SessionList로 이동하는 플로우입니다. 지금 코드는 Navigation을 활용하기 전으로 롤백되어있고 결국 navigation 처리를 위한 새로운 Route가 추가된 경우입니다. 참고 개인 블로그에 작성한 내용을 기반합니다 https://thdev.tech/architecture/2025/06/02/Android-Architecture-new-02/ |
|
네비게이션 로직도 오늘중에 개선 해두겠습니다. 감사합니다. |
@Composable
private fun InternalLaunchedRouter(
navHostController: NavHostController,
routerViewModel: RouterViewModel = hiltViewModel(),
) {
val lifecycleOwner = LocalLifecycleOwner.current
LaunchedEffect(routerViewModel, lifecycleOwner) {
lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) {
routerViewModel.sideEffect.collectLatest { sideEffect ->
when (sideEffect) {
is RouteSideEffect.NavigateBack -> {
navHostController.popBackStack()
}
is RouteSideEffect.Navigate -> {
navHostController.navigate(sideEffect.route) {
navHostController.graph.findStartDestination().route?.let {
popUpTo(it) { // <- 이 부분 때문에 항상 홈 탭으로 돌아감
saveState = sideEffect.saveState
}
}
restoreState = sideEffect.saveState
}
}
}
}
}
}
}라우터 구현중에 popUpTo() 가 있어서 |
is RouteSideEffect.Navigate -> {
navHostController.navigate(sideEffect.route) {
if (sideEffect.popUpToStart) {
navHostController.graph.findStartDestination().route?.let {
popUpTo(it) {
saveState = sideEffect.saveState
}
}
}
restoreState = sideEffect.saveState
}
}
해당 플래그를 추가하면 연쇄적으로 네비게이터 관련 코드에 같은 플래그가 추가 되어야 하는데 (복잡성 이슈...?)
뭔가 더 스마트하고 심플한 방법을 생각해봐야겠군요 🤔 |
|
Navigator 사용시 백스택이 항상 Start Destination (홈 탭) 으로 초기화 되는 동작은 현재 필요한 부분이 없기 때문에 Navigator 구현의 |
| onShowErrorSnackBar: (throwable: Throwable?) -> Unit, | ||
| ) { | ||
| composable<RouteSession> { | ||
| val sessionId: String? = it.toRoute<Route.SessionList>().sessionId |
There was a problem hiding this comment.
이거 안돌아가지 않나요? 확인을...
viewModelScope.launch {
navigator.navigate(
route = RouteSession(sessionId = session.id),
)
}
There was a problem hiding this comment.
라우터 api 모듈 의존성 추가해서 수정했습니다!
package com.droidknights.app.core.navigation
sealed interface Route {
@Serializable
data class SessionList(val sessionId: String? = null) : Route
}
package com.droidknights.app.feature.session.api
@Serializable
data class RouteSession(val sessionId: String? = null) : Route
아마 이전엔 데이터 구조가 같다보니 @serializable 에 의해 직렬화 되면서 동작했었나봐요.
Issue
Overview (Required)
KnightsColor.Blue03)으로 하이라이팅 된다.Screenshot
2025-06-02.10.14.09.mov