-
Notifications
You must be signed in to change notification settings - Fork 0
[feat/#12] SnackBar 컴포넌트 구현 #13
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
Conversation
vvan2
left a comment
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.
고생하셨습니다!! 컴포져블 함수 부분에서는 크게 수정할 사항은 없는 것 같고,
snackbarHostState 를 어떤식으로 관리해서 사용하는게 더 좋을지 고민이 돼서 해당 부분 코리 남겼습니다. 같이 의논해보는게 좋을 것 같습니다
| .fillMaxWidth() | ||
| .clip(RoundedCornerShape(12.dp)) | ||
| .background(KieroTheme.colors.gray900) | ||
| .noRippleClickable { } |
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.
p1) 클릭에 대한 의도가 없다면 noRippleClickable을 제거 해도 될 것 같습니다!!
또한, snackbar 가 알림의 역할을 하는 컴포넌트이기 때문에, 추가적인 클릭에 대한 인터렉션이 없을 거 같아 제거하는게 좋아보입니당
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.
혹시나 사용자가 스낵바를 클릭해서 하단 UI로 이벤트가 투과되어 의도치 않게 동작이 발생할 수 있지 않을까? 생각이 들어서 이벤트 소비 목적으로 추가했었습니다..
하지만 현재는 알림 역할만 하고 클릭 요소는 아니니 제거하는게 좋겠네요!
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.
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,
)
}이러한 구조도 나쁘지 않을 것 같다는 생각인데, 팀원들의 의견을 듣고 싶네용
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.
확장성까지 고려했을 때, CompositionLocal 방식도 괜찮을 것 같습니다!
현재 파라미터 전달 방식은 모든 중간 레이어에 파라미터를 전달해야 하고,
Dialog 등이 추가되면 파라미터가 계속 늘어날 수 있어서
CompositionLocal 방식으로 개선하는 것도 좋아보입니다!
다만, 어떤 화면이 무엇을 사용하는지 파악이 어려울 수도 있어서 이부분은 조금 조심해야할 것 같아요 !
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.
CompositionLocalProvider 이 더 나을 것 같습니다 !
현재 어디서 어느 뷰에서 사용될지는 모르겠으나 MainAppState 부터 시작해서 feature의 기능 발생까지 내려 가야하는데
사용예시만 보더라도 하나의 메세지 발생을 위해 약6~7개의 파라미터를 내려주게 됩니다
이 방식은 보일러플레이트가 존재할 뿐더러 파라미터로 넘어가는 단계마다 비교 비용이 발생하게됩니다
따라서 CompositionLocalProvider을 적용해서 중앙 집중화와 일관성을 통해 단일 진실 소스를 보장하게 하는 방식이 더 좋을 것 같다고 생각합니다.
다만, 어떤 화면이 무엇을 사용하는지 파악이 어려울 수도 있어서 이부분은 조금 조심해야할 것 같아요 !
우려하시는 부분은 이해합니다 파라미터로 흐름을 파악 가능 했어서 우려하신 것 같은데
오히려 파라미터를 위에서 사용하는 뷰까지 내릴 동안 중간 뷰들이 가지는 파라미터 드릴링 방식이 더 구현의 파악이 어렵게 만드는 것 같아요
그래서 필요한 곳에서만 공구함에서 딱 꺼내 쓰는 느낌으로 사용해서 해당 컴포저블의 관심사를 파악하는데 더 직관적일 것 같습니다
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.
CompositionLocalProvider를 활용하면 현재보다 훨씬 깔끔하게 관리될 것 같아 좋은 것 같습니다!
반영해볼게요.
| import com.kiero.core.designsystem.theme.KieroTheme | ||
|
|
||
| @Composable | ||
| fun KieroSnackbar( |
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) 굳굳슨, 공통 컴포넌트 작업시에 컴포져블 함수 네이밍은 Kiero+컴포넌트명으로 통일할까요?
서비스명을 안붙히고 역할+컴포넌트명 으로 하는 상황도 많이 봤어서 통일하고 가는게 좋을 것 같습니당
@seungjae708 @sonms @dmp100
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.
괜찮다고 생각합니다 ! 공통 컴포넌트로 쓰기엔 적합한거같아요 !
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.
공통이라는 것을 한눈에 알기쉬워서 키어로+컴포넌트명으로 통일하는 것으로 하는 것이 좋을 것 같아요!
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun KieroSnackbarPreview() { |
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.
👍
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.
수고하셨습니다 ! Appstate로 관리하는 방식은 저도 첨이라 조금 어렵긴했는데, 훌륭하게 잘 만들어주신것 같아요 !
SnackbarHostState의 중복호출 부분만 조금 생각해보시는게 어떤지 의견 남깁니다 !
- 제가실수로 제꺼 Request를 눌러버려서 awaiting requested review from dmp100이라고 뜨네요ㅎㅎ 어느정도 수정하셨으면 어푸로 새로남기겠습니다 !
| import com.kiero.core.designsystem.theme.KieroTheme | ||
|
|
||
| @Composable | ||
| fun KieroSnackbar( |
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 showSnackbar(message: String) { | ||
| coroutineScope.launch { | ||
| snackbarHostState.showSnackbar( | ||
| message = message, | ||
| duration = SnackbarDuration.Short | ||
| ) | ||
| } | ||
| } | ||
|
|
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.
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로 남겨놔도 좋을것 같다고 생각합니다 !
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.
확장성까지 고려했을 때, CompositionLocal 방식도 괜찮을 것 같습니다!
현재 파라미터 전달 방식은 모든 중간 레이어에 파라미터를 전달해야 하고,
Dialog 등이 추가되면 파라미터가 계속 늘어날 수 있어서
CompositionLocal 방식으로 개선하는 것도 좋아보입니다!
다만, 어떤 화면이 무엇을 사용하는지 파악이 어려울 수도 있어서 이부분은 조금 조심해야할 것 같아요 !
sonms
left a comment
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.
캬! 고생하셨습니다~ 제가 남긴 리뷰 천천히 확인 해주세용
| import com.kiero.core.designsystem.theme.KieroTheme | ||
|
|
||
| @Composable | ||
| fun KieroSnackbar( |
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.
공통이라는 것을 한눈에 알기쉬워서 키어로+컴포넌트명으로 통일하는 것으로 하는 것이 좋을 것 같아요!
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.
CompositionLocalProvider 이 더 나을 것 같습니다 !
현재 어디서 어느 뷰에서 사용될지는 모르겠으나 MainAppState 부터 시작해서 feature의 기능 발생까지 내려 가야하는데
사용예시만 보더라도 하나의 메세지 발생을 위해 약6~7개의 파라미터를 내려주게 됩니다
이 방식은 보일러플레이트가 존재할 뿐더러 파라미터로 넘어가는 단계마다 비교 비용이 발생하게됩니다
따라서 CompositionLocalProvider을 적용해서 중앙 집중화와 일관성을 통해 단일 진실 소스를 보장하게 하는 방식이 더 좋을 것 같다고 생각합니다.
다만, 어떤 화면이 무엇을 사용하는지 파악이 어려울 수도 있어서 이부분은 조금 조심해야할 것 같아요 !
우려하시는 부분은 이해합니다 파라미터로 흐름을 파악 가능 했어서 우려하신 것 같은데
오히려 파라미터를 위에서 사용하는 뷰까지 내릴 동안 중간 뷰들이 가지는 파라미터 드릴링 방식이 더 구현의 파악이 어렵게 만드는 것 같아요
그래서 필요한 곳에서만 공구함에서 딱 꺼내 쓰는 느낌으로 사용해서 해당 컴포저블의 관심사를 파악하는데 더 직관적일 것 같습니다
| fun showSnackbar(message: String) { | ||
| coroutineScope.launch { | ||
| snackbarHostState.showSnackbar( | ||
| message = message, | ||
| duration = SnackbarDuration.Short | ||
| ) | ||
| } | ||
| } |
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.
p1: MainAppState의 역할이 커져 관심사 분리가 꺠질 것 같습니다
MainAppState의 현재는 본질적인 역할로 네비게이션 관련 상태와 관련 조작 로직과 앱의 전체 상태를 가집니다
스낵바의 표시 역할까지 존재한다면? 다이얼로그 , 토스트 상태 표시까지 다 들어가게 될 것 같아요 그럼 엄청 비대해질 것 같습니다
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.
개인적인 궁금증이긴합니다만
관심사 분리 시 민성님만의 "높은 응집도"의 판단 기준이 궁금합니다.
예를 들어, 코틀린 파일별로 하나의 명확한 책임이 들어가야하는지 생각하시는지? 혹은 MainAppState처럼 클래스 하나에 여러 기능이 있을 때, 어느 시점에 별도 파일로 분리해야 한다고 판단하시나요 ?
구체적인 분리 기준(라인 수, 책임 개수 등)이 있으실까요?
@vvan2 @sonms @seungjae708
다른분들도 어떤 관점을 갖고있는지도 궁금합니다 !
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.
음 저는 우선 파일의 역할과 목적을 생각합니다
사실 이상향이긴한데..😅
제가 만든 파일이 무엇을 하는 녀석인지 딱 보고 정의가 불가능하다면 분리하는게 좋다고 생각합니다
라인수도 하나의 파싱로직이 500줄이 넘어가도 파싱의 역할 1개를 가지고 있기때문에 분리하지 않아도 될 것 같습니다
또는 MainAppState를 예를 들어 수정해야하는데 네크워크 로직이나 바텀네비게이션 UI가 바뀌어서 수정된다면? 이 하나의 파일은 기존에 네비게이션과 관련 조작 로직, 앱 전체 상태(오프라인 상태)등을 따져 안에 네크워크, UI 의존성이 모두 들어있기 때문에 수정되는 거 겠죠?
그럼 분리해야한다고 생각해요
변경의 이유와 그 변경 후의 다른 파일도 수정해야하는 파장이 존재한다면? 분리해야하지만 ..!!!
사실 어렵긴해서 비슷한 역할을 가진다면 얼추 혼합해서 사용해도 큰 문제는 없을 것 같습니다 아마두..
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.
좋은 질문이네요, 어제 일정을 마치고 코리 달러 왔습니다. (키다닥)
저도 저번 프로젝트 세팅부터 네비게이션에 달린 코드리뷰를 보면서 파일을 어떻게 나눌 것인지, 해당 파일은 어떤 역할을 하는지, 어느 위치에 둘지 등 고민해본 것 같습니다.
제가 생각하는 "높은 응집도"의 판단 기준
저는 의도와 범위가 핵심이라고 생각합니다.
음.. 저희가 많이 접한 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개의 틀로 나눠놓았긴 때문에 범위를 같이 고려하면 좋은 분리가 될거라 생각하고, 코드의 길이는 의도가 명확하고 관심사 분리가 잘 되어있다면 크게 문제 되지 않는다 생각합니다
물론, 저도 머리로는 이해하는데 작성하다 보면 응집도가 높거나 하는 경우가 아직 많아서 민성님께 열심히 두들겨 맞고 있는중입니다. 감사합니다
| val snackbarHostState: SnackbarHostState, | ||
| val coroutineScope: CoroutineScope, |
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.
p1: coroutine에 val 을 붙여 멤버변수로 만들게 될 경우 다른 곳에서 appState.coroutineScope... 처럼 이 스코프에 접근할 수 있게 되어 캡슐화가 위반됩니다
현재 Scaffold 구조상 내부적으로 바텀바 높이를 계산해 그 위에 스낵바를 띄워주기 때문에 승재님 말씀처럼 추후 디자인 확정 시 높이 수정하시면 될 것 같습니다! |
sonms
left a comment
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.
굿굿 어푸푸
dmp100
left a comment
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.
아주 좋아용
vvan2
left a comment
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.
어푸푸!!
ISSUE
❗ WORK DESCRIPTION
사용예시
KieroNavHost 상태 주입
authNavGraph( navController = appState.navController, paddingValues = paddingValues, onShowSnackbar = appState::showSnackbar, // 추가 navigateUp = appState::navigateUp, navigateToParent = appState::navigateToParent, navigateToKid = appState::navigateToKid, )AuthNavigation 이벤트 정의 및 스크린으로의 이벤트 전파
AuthScreen 사용자의 인터랙션을 이벤트로 변환
📢 TO REVIEWERS
그래서 Row가 아닌 Box를 이용해서 구현하였습니다.
📸 SCREENSHOT
Screen_Recording_20260108_184643_Kiero.mp4