Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package com.kiero.core.designsystem.component

import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import com.kiero.core.common.extension.noRippleClickable
import com.kiero.core.designsystem.theme.KieroTheme

@Composable
fun KieroSnackbar(
Copy link
Member

Choose a reason for hiding this comment

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

p3) 굳굳슨, 공통 컴포넌트 작업시에 컴포져블 함수 네이밍은 Kiero+컴포넌트명으로 통일할까요?
서비스명을 안붙히고 역할+컴포넌트명 으로 하는 상황도 많이 봤어서 통일하고 가는게 좋을 것 같습니당
@seungjae708 @sonms @dmp100

Copy link
Collaborator

Choose a reason for hiding this comment

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

괜찮다고 생각합니다 ! 공통 컴포넌트로 쓰기엔 적합한거같아요 !

Copy link
Member

Choose a reason for hiding this comment

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

공통이라는 것을 한눈에 알기쉬워서 키어로+컴포넌트명으로 통일하는 것으로 하는 것이 좋을 것 같아요!

message: String,
modifier: Modifier = Modifier,
) {
Box(
modifier = modifier
.fillMaxWidth()
.clip(RoundedCornerShape(12.dp))
.background(KieroTheme.colors.gray900)
.noRippleClickable { }
Copy link
Member

Choose a reason for hiding this comment

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

p1) 클릭에 대한 의도가 없다면 noRippleClickable을 제거 해도 될 것 같습니다!!
또한, snackbar 가 알림의 역할을 하는 컴포넌트이기 때문에, 추가적인 클릭에 대한 인터렉션이 없을 거 같아 제거하는게 좋아보입니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

혹시나 사용자가 스낵바를 클릭해서 하단 UI로 이벤트가 투과되어 의도치 않게 동작이 발생할 수 있지 않을까? 생각이 들어서 이벤트 소비 목적으로 추가했었습니다..

하지만 현재는 알림 역할만 하고 클릭 요소는 아니니 제거하는게 좋겠네요!

.padding(16.dp),
contentAlignment = Alignment.Center
) {
Text(
text = message,
style = KieroTheme.typography.regular.body4,
color = KieroTheme.colors.schedule1,
textAlign = TextAlign.Center
)
}
}

@Preview(showBackground = true)
@Composable
private fun KieroSnackbarPreview() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

KieroTheme {
Column(
modifier = Modifier.fillMaxSize(),
verticalArrangement = Arrangement.Bottom,
horizontalAlignment = Alignment.CenterHorizontally,
) {
KieroSnackbar(
message = "이 메세지는 토스트 메세지 입니다",
modifier = Modifier.padding(16.dp),
)
}
}
}
Copy link
Member

@vvan2 vvan2 Jan 8, 2026

Choose a reason for hiding this comment

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

p1) AppState 를 통해 navigation 상태 관리를 하는게 저도 처음이라, 사실 보고 좀 생각을 해봤습니다.
snackbar와 같은 경우 MainAppState 에서 파라미터로 전달하는 방식도 있긴한데, 다른 프로젝트를 좀 찾아보니까
전역적으로 compositionlocal 을 사용해서 필요한 곳에 만 사용하는 방법도 많더라구요!!
사용예시로 올려주신 방법처럼 host-> graph -> route -> screen 으로 전달 하게 되면 사용하는 모든 화면에 명시적으로 전달해야 되는 리소스가 생기지만,
compositionLocalprovider 로 필요한 screen 에서만 선언해주면 편리할 것 같습니다.

@Composable
fun AuthScreen(
    paddingValues: PaddingValues,
    navigateUp: () -> Unit,
    navigateToParent: () -> Unit,
    navigateToKid: () -> Unit,
    modifier: Modifier = Modifier,
) {
    val showSnackbar = SnackbarController.Local.current

또한, snackBar 와 더불어 저희 프로젝트에서는 dialog 또한 사용하고 있는데 추후 사용될 dialog 까지 생각하면

CompositionLocalProvider(
    // 여기서 전역으로 provide 해주고
    AppController.SnackbarLocal provides appState::showSnackbar,
    AppController.DialogLocal provides dialogController::show,
) {
    KieroNavHost(
        // NavHost 내부에서는 navigation 로직만을 명시적 전달
        navigateToParent = appState::navigateToParent,
        navigateToKid = appState::navigateToKid,
    )
}

이러한 구조도 나쁘지 않을 것 같다는 생각인데, 팀원들의 의견을 듣고 싶네용

Copy link
Collaborator

Choose a reason for hiding this comment

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

확장성까지 고려했을 때, CompositionLocal 방식도 괜찮을 것 같습니다!

현재 파라미터 전달 방식은 모든 중간 레이어에 파라미터를 전달해야 하고,
Dialog 등이 추가되면 파라미터가 계속 늘어날 수 있어서
CompositionLocal 방식으로 개선하는 것도 좋아보입니다!
다만, 어떤 화면이 무엇을 사용하는지 파악이 어려울 수도 있어서 이부분은 조금 조심해야할 것 같아요 !

Copy link
Member

Choose a reason for hiding this comment

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

CompositionLocalProvider 이 더 나을 것 같습니다 !

현재 어디서 어느 뷰에서 사용될지는 모르겠으나 MainAppState 부터 시작해서 feature의 기능 발생까지 내려 가야하는데
사용예시만 보더라도 하나의 메세지 발생을 위해 약6~7개의 파라미터를 내려주게 됩니다

이 방식은 보일러플레이트가 존재할 뿐더러 파라미터로 넘어가는 단계마다 비교 비용이 발생하게됩니다

따라서 CompositionLocalProvider을 적용해서 중앙 집중화와 일관성을 통해 단일 진실 소스를 보장하게 하는 방식이 더 좋을 것 같다고 생각합니다.

@dmp100

다만, 어떤 화면이 무엇을 사용하는지 파악이 어려울 수도 있어서 이부분은 조금 조심해야할 것 같아요 !

우려하시는 부분은 이해합니다 파라미터로 흐름을 파악 가능 했어서 우려하신 것 같은데
오히려 파라미터를 위에서 사용하는 뷰까지 내릴 동안 중간 뷰들이 가지는 파라미터 드릴링 방식이 더 구현의 파악이 어렵게 만드는 것 같아요
그래서 필요한 곳에서만 공구함에서 딱 꺼내 쓰는 느낌으로 사용해서 해당 컴포저블의 관심사를 파악하는데 더 직관적일 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CompositionLocalProvider를 활용하면 현재보다 훨씬 깔끔하게 관리될 것 같아 좋은 것 같습니다!
반영해볼게요.

Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.kiero.presentation.main.navigation

import androidx.compose.material3.SnackbarDuration
import androidx.compose.material3.SnackbarHostState
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Stable
import androidx.compose.runtime.remember
Expand Down Expand Up @@ -27,12 +29,14 @@ import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn
import kotlinx.coroutines.launch


@Stable
class MainAppState(
val navController: NavHostController,
coroutineScope: CoroutineScope,
val snackbarHostState: SnackbarHostState,
val coroutineScope: CoroutineScope,
Copy link
Member

Choose a reason for hiding this comment

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

p1: coroutine에 val 을 붙여 멤버변수로 만들게 될 경우 다른 곳에서 appState.coroutineScope... 처럼 이 스코프에 접근할 수 있게 되어 캡슐화가 위반됩니다

) {
private val currentDestination = navController.currentBackStackEntryFlow
.map { it.destination }
Expand Down Expand Up @@ -86,6 +90,15 @@ class MainAppState(
initialValue = false
)

fun showSnackbar(message: String) {
coroutineScope.launch {
snackbarHostState.showSnackbar(
message = message,
duration = SnackbarDuration.Short
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

p1: MainAppState의 역할이 커져 관심사 분리가 꺠질 것 같습니다

MainAppState의 현재는 본질적인 역할로 네비게이션 관련 상태와 관련 조작 로직과 앱의 전체 상태를 가집니다

스낵바의 표시 역할까지 존재한다면? 다이얼로그 , 토스트 상태 표시까지 다 들어가게 될 것 같아요 그럼 엄청 비대해질 것 같습니다

Copy link
Collaborator

@dmp100 dmp100 Jan 9, 2026

Choose a reason for hiding this comment

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

개인적인 궁금증이긴합니다만
관심사 분리 시 민성님만의 "높은 응집도"의 판단 기준이 궁금합니다.
예를 들어, 코틀린 파일별로 하나의 명확한 책임이 들어가야하는지 생각하시는지? 혹은 MainAppState처럼 클래스 하나에 여러 기능이 있을 때, 어느 시점에 별도 파일로 분리해야 한다고 판단하시나요 ?
구체적인 분리 기준(라인 수, 책임 개수 등)이 있으실까요?
@vvan2 @sonms @seungjae708

다른분들도 어떤 관점을 갖고있는지도 궁금합니다 !

Copy link
Member

Choose a reason for hiding this comment

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

음 저는 우선 파일의 역할과 목적을 생각합니다
사실 이상향이긴한데..😅

제가 만든 파일이 무엇을 하는 녀석인지 딱 보고 정의가 불가능하다면 분리하는게 좋다고 생각합니다
라인수도 하나의 파싱로직이 500줄이 넘어가도 파싱의 역할 1개를 가지고 있기때문에 분리하지 않아도 될 것 같습니다

또는 MainAppState를 예를 들어 수정해야하는데 네크워크 로직이나 바텀네비게이션 UI가 바뀌어서 수정된다면? 이 하나의 파일은 기존에 네비게이션과 관련 조작 로직, 앱 전체 상태(오프라인 상태)등을 따져 안에 네크워크, UI 의존성이 모두 들어있기 때문에 수정되는 거 겠죠?
그럼 분리해야한다고 생각해요

변경의 이유와 그 변경 후의 다른 파일도 수정해야하는 파장이 존재한다면? 분리해야하지만 ..!!!

사실 어렵긴해서 비슷한 역할을 가진다면 얼추 혼합해서 사용해도 큰 문제는 없을 것 같습니다 아마두..

Copy link
Member

Choose a reason for hiding this comment

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

좋은 질문이네요, 어제 일정을 마치고 코리 달러 왔습니다. (키다닥)
저도 저번 프로젝트 세팅부터 네비게이션에 달린 코드리뷰를 보면서 파일을 어떻게 나눌 것인지, 해당 파일은 어떤 역할을 하는지, 어느 위치에 둘지 등 고민해본 것 같습니다.
제가 생각하는 "높은 응집도"의 판단 기준
저는 의도와 범위가 핵심이라고 생각합니다.
음.. 저희가 많이 접한 MVVM 패턴을 예로 들면 간단하게

Composable: UI 렌더링만 담당
ViewModel: 상태 관리와 비즈니스 로직 처리

이런식으로 구분할 수 있겠죠. 이처럼 각 레이어의 의도가 명확하게 구분되어야 합니다.
질문주신 MainAppState 에 대해 얘기를 해보자면
MainAppState를 수정할 때 네트워크 상태 때문일 수도 있고, 바텀 네비게이션 UI 변경 때문일 수도 있고,
스낵바 표시 로직 때문일 수도 있다면 이러한 상황이 여러 관심사가 섞여 있는 구조 같습니다.

@.. 음 저번 코리 태그 어캐하지, 암튼
저번 코리를 보시면서 생각해보면 좋을 것 같습니다. 현재 프로젝트에서는 Navigation 구조를

NavHost -> Graph들의 중앙 관리
NavGraph -> 각 Feature별 화면 전환 정의
MainAppState -> Navigation 관련 상태 관리
BottomNavBar -> UI 렌더링
MainTab ->Tab 정의 

이런식으로 각 파일의 역할을 잡아보았습니다
BottomNavBar 는 UI 렌더링을 하며 네비게이션 상태가 어떻게 관리되는지, 다른 화면으로 어떻게 전환되는지는 알 필요가 없으며,
NavHost는 전체 Graph 구조를 관리하고 AuthGraph, ParentGraph, KidGraph 등을 포함하는 컨테이너 역할을 한다고 생각합니다. 그렇다면 각 Graph 내부의 구체적인 화면 전환 로직은 알 필요가 없죠

마지막으로 현재 상황에 맞게 분리를 해야한다면
MainAppState는 Navigation 관련 상태 및 제어 로직 관리의 역할을 한다고 생각하는데
snackbar 관련 함수가 MainAppState 에 들어있는게 맞나? 입니다

    // 이 놈
    fun showSnackbar(message: String) { ... }
}

MainAppState의 본질적 역할이 Navigation 상태 관리이 기 때문에 snackbar 관련 함수는 관심사 분리가 필요해 보입니다.
앞서 코리게 달았듯이, 여기에 Snackbar, Dialog, Toast 같은 전역 UI 컴포넌트의 표시 로직까지 추가되면 이 클래스는 Navigation만 관리하는가? UI 알림도 관리하는가?" 혼동이 올 수도 있고 굉장히 비대해 질 수 있다 생각합니다. 추가적으로 Navigation 로직 변경과 Snackbar 표시 방식 변경이 같은 파일을 건드리는 상황이 있을 수도 있기 떄문에

MainAppState: Navigation 상태만 관리 → 명확한 단일 책임
SnackbarController: Snackbar 표시만 관리 → 명확한 단일 책임
CompositionLocal: 전역 UI 컴포넌트들을 한 곳에서 제공 → Core/Common 레벨의 관심사

로 분리하는 식이 좋다고 생각합니다.
결론으로.. 저의 판단 기준은
민성님이 말씀하신 파일/클래스의 역할을 한 문장으로 설명할 수 없을 때나, 혹은 한 부분의 변경이 관련 없는 다른 부분에 영향을 줄 때 분리해야 한다 생각합니다. 이 때 저희는 parent, auth, kid 라는 3개의 틀로 나눠놓았긴 때문에 범위를 같이 고려하면 좋은 분리가 될거라 생각하고, 코드의 길이는 의도가 명확하고 관심사 분리가 잘 되어있다면 크게 문제 되지 않는다 생각합니다

물론, 저도 머리로는 이해하는데 작성하다 보면 응집도가 높거나 하는 경우가 아직 많아서 민성님께 열심히 두들겨 맞고 있는중입니다. 감사합니다


Copy link
Collaborator

Choose a reason for hiding this comment

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

p2) SnackbarHostState는 하나의 스낵바만 표시하므로 새로운 요청이 오면 이전 스낵바가 즉시 사라질거 같은데,
예를들면 A화면의 appState.showSnackbar("파일이 저장되었습니다") 를하고 0.5초후에 B화면에서 appState.showSnackbar("일정이 추가되었습니다")가 진행되면 파일이 저장되었습니다"는 표시되지 않고, "일정이 추가되었습니다"만 표시될 수도있을거 같다고 생각이 들어요 !

fun showSnackbar(message: String) {
    coroutineScope.launch {
        // TODO: 빠른 연속 호출 시 명시적 dismiss 고려
        //  snackbarHostState.currentSnackbarData?.dismiss()
        snackbarHostState.showSnackbar(
            message = message,
            duration = SnackbarDuration.Short
        )
    }
}

이전 스낵바 취소와같이 중복 호출의 경우에 대한 로직도 Todo로 남겨놔도 좋을것 같다고 생각합니다 !

fun navigateToParent() {
navController.navigate(ParentGraph) {
popUpTo(AuthGraph) { inclusive = true }
Expand Down Expand Up @@ -156,11 +169,13 @@ class MainAppState(
@Composable
fun rememberMainAppState(
navController: NavHostController = rememberNavController(),
snackbarHostState: SnackbarHostState = remember { SnackbarHostState() },
coroutineScope: CoroutineScope = rememberCoroutineScope(),
): MainAppState {
return remember(navController, coroutineScope) {
return remember(navController, snackbarHostState, coroutineScope) {
MainAppState(
navController = navController,
snackbarHostState = snackbarHostState,
coroutineScope = coroutineScope
)
}
Expand Down
20 changes: 9 additions & 11 deletions app/src/main/java/com/kiero/presentation/main/screen/MainScreen.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material3.Scaffold
import androidx.compose.material3.SnackbarHost
import androidx.compose.material3.SnackbarHostState
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.unit.dp
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.kiero.core.designsystem.component.KieroSnackbar
import com.kiero.presentation.main.navigation.KidMainTab
import com.kiero.presentation.main.navigation.KieroNavHost
import com.kiero.presentation.main.navigation.MainAppState
Expand All @@ -22,20 +21,14 @@ import kotlinx.collections.immutable.toImmutableList

@Composable
fun MainRoute(
appState: MainAppState = rememberMainAppState(),
appState: MainAppState = rememberMainAppState()
) {
val snackBarHostState = remember { SnackbarHostState() }

MainScreen(
appState = appState,
snackBarHostState = snackBarHostState,
)
MainScreen(appState = appState)
}

@Composable
fun MainScreen(
appState: MainAppState,
snackBarHostState: SnackbarHostState,
modifier: Modifier = Modifier,
) {
val showParentBottomBar by appState.showParentBottomBar.collectAsStateWithLifecycle()
Expand All @@ -59,7 +52,12 @@ fun MainScreen(
Scaffold(
modifier = modifier.fillMaxSize(),
snackbarHost = {
SnackbarHost(hostState = snackBarHostState)
SnackbarHost(hostState = appState.snackbarHostState) { data ->
KieroSnackbar(
message = data.visuals.message,
modifier = Modifier.padding(16.dp)
)
}
},
bottomBar = {
MainBottomBar(
Expand Down
Loading