공통 바텀시트 구조 구현 #46
공통 바텀시트 구조 구현 #46
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can get early access to new features in CodeRabbit.Enable the 📝 WalkthroughWalkthrough이 PR은 디자인 시스템과 UI 패키지 재배치, 의존성 조정, 그리고 다수의 신규 UI 컴포넌트 추가를 포함합니다. Toast 관련 패키지가 Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/main/src/main/java/com/twix/home/HomeViewModel.kt (1)
59-69:shiftWeek에서 TODAY 액션 시 중복 상태 업데이트가 발생합니다.
WeekNavigation.TODAY액션일 때updateDate(newReference)가 이미referenceDate와selectedDate를 모두 업데이트하는데, 이후 line 68에서 다시referenceDate를 동일한 값으로 설정합니다. 이로 인해 불필요한 상태 업데이트가 두 번 발생하고, 구독자들에게 중복 이벤트가 전달될 수 있습니다.🔧 중복 업데이트 제거를 위한 수정 제안
private fun shiftWeek(action: WeekNavigation) { val newReference = when (action) { WeekNavigation.NEXT -> currentState.referenceDate.plusWeeks(1) WeekNavigation.PREVIOUS -> currentState.referenceDate.minusWeeks(1) WeekNavigation.TODAY -> LocalDate.now() } if (currentState.referenceDate == newReference) return - if (action == WeekNavigation.TODAY) updateDate(newReference) - reduce { copy(referenceDate = newReference) } + if (action == WeekNavigation.TODAY) { + updateDate(newReference) + } else { + reduce { copy(referenceDate = newReference) } + } }
🤖 Fix all issues with AI agents
In `@feature/main/src/main/java/com/twix/home/HomeScreen.kt`:
- Around line 97-99: The button's onClick in HomeScreen.kt is left as a TODO
which causes a non-responsive UX; either disable the button temporarily or wire
it to a minimal action: set the Button/Clickable's enabled=false (or add a
visual disabled state) OR call the existing navigation/UI handler (e.g., invoke
navigateToAddGoal(...) or showAddGoalBottomSheet() if those exist) from the
onClick lambda so pressing it produces an expected effect; update the onClick in
HomeScreen (or add a new navigateToAddGoal callback parameter to HomeScreen if
needed) to connect the action and ensure any required parameters or coroutine
scope are provided.
In `@feature/main/src/main/java/com/twix/main/MainScreen.kt`:
- Around line 41-43: HomeViewModel is being instantiated inside the MainScreen
composable via koinViewModel(), which can recreate the VM on recomposition and
blurs ownership between MainRoute and MainScreen; move ViewModel creation to
MainRoute and hoist its state into MainScreen by creating the HomeViewModel in
MainRoute (use koinViewModel() there), expose calendarState (from
homeViewModel.calendarState.collectAsStateWithLifecycle()) and any event
handlers or showCalendarBottomSheet state as parameters, then update MainScreen
signature to accept a HomeViewModel or the specific states and callbacks so
MainRoute owns the VM and MainScreen becomes a pure, testable UI.
🧹 Nitpick comments (17)
core/design-system/src/main/res/drawable/ic_arrow_m_right.xml (1)
1-13: Dark Mode 지원을 위해 하드코딩된 색상 사용 검토 필요현재
strokeColor="#171717"이 하드코딩되어 있어 Dark Mode에서 아이콘이 배경과 구분되지 않을 수 있습니다.왜 문제가 되는지:
#171717은 거의 검정색에 가까운 색상입니다- Dark Mode의 어두운 배경에서 이 아이콘은 거의 보이지 않게 됩니다
- 디자인 시스템 가이드라인에 따르면 Dark Mode 지원 여부를 고려해야 합니다
개선 방법:
테마 속성이나 색상 리소스를 사용하여 Light/Dark 모드에 자동으로 대응하도록 수정하는 것을 권장합니다.♻️ 권장 수정 방안
<path - android:strokeColor="#171717" + android:strokeColor="@color/icon_primary" .../>또는 테마 속성 사용:
<path - android:strokeColor="#171717" + android:strokeColor="?attr/colorOnSurface" .../>
@color/icon_primary를 사용할 경우,values/colors.xml과values-night/colors.xml에 각각 적절한 색상을 정의해야 합니다.core/design-system/src/main/res/drawable/ic_arrow_m_left.xml (1)
1-13: ic_arrow_m_right.xml과 동일한 Dark Mode 이슈
ic_arrow_m_right.xml과 마찬가지로strokeColor="#171717"이 하드코딩되어 있습니다. 두 아이콘 모두 동일한 방식으로 색상 리소스 또는 테마 속성을 사용하도록 수정하면, 일관성 있게 Dark Mode를 지원할 수 있습니다.아이콘 쌍이 대칭적으로 잘 구현되어 있는 점은 좋습니다! 👍
♻️ 권장 수정 방안
<path - android:strokeColor="#171717" + android:strokeColor="@color/icon_primary" .../>core/design-system/src/main/java/com/twix/designsystem/components/toast/ToastManager.kt (1)
8-28: ToastManager 구현이 깔끔합니다.
SharedFlow를 활용한 토스트 관리 패턴이 적절합니다.extraBufferCapacity = 16과DROP_OLDEST정책은 일반적인 토스트 UX에 합리적인 설정입니다.show()와tryShow()두 가지 API를 제공하여 사용 상황에 맞게 선택할 수 있도록 한 점이 좋습니다.- 한글 주석으로 각 메서드의 사용 시나리오를 설명한 것도 유지보수에 도움이 됩니다.
선택적 개선 제안: 테스트 용이성을 높이기 위해 인터페이스를 분리하는 것을 고려해볼 수 있습니다. 현재 구현도 충분히 간결하지만, 향후 테스트에서 mock이 필요할 경우 인터페이스 분리가 도움이 될 수 있습니다. 현 시점에서는 필수가 아니므로 참고만 해주세요.
💡 인터페이스 분리 예시 (선택 사항)
interface ToastManager { val toasts: SharedFlow<ToastData> suspend fun show(data: ToastData) fun tryShow(data: ToastData): Boolean } class DefaultToastManager : ToastManager { private val _toasts = MutableSharedFlow<ToastData>( replay = 0, extraBufferCapacity = 16, onBufferOverflow = BufferOverflow.DROP_OLDEST, ) override val toasts: SharedFlow<ToastData> = _toasts override suspend fun show(data: ToastData) { _toasts.emit(data) } override fun tryShow(data: ToastData): Boolean = _toasts.tryEmit(data) }core/design-system/src/main/java/com/twix/designsystem/components/bottomsheet/AdaptiveSheetList.kt (1)
14-51: 재사용 가능한 적응형 리스트 컴포넌트입니다! 좋은 설계입니다. 👍아이템 수에 따라
Column과LazyColumn을 자동으로 전환하는 패턴은 적은 아이템에서는 단순함을, 많은 아이템에서는 가상화를 통한 성능을 제공합니다.다만 두 가지 개선 포인트가 있습니다:
Column 브랜치에서 key 미사용:
LazyColumn에서는key를 사용하지만,Column브랜치의forEach에서는 key 정보가 활용되지 않습니다. 아이템 순서가 변경되면 불필요한 recomposition이 발생할 수 있습니다.Preview 누락: 디자인 시스템 리뷰 가이드에 따르면 다양한 상태에 대한 Preview가 제공되어야 합니다.
♻️ Column에서 key 활용 및 Preview 추가 제안
Column( modifier = modifier .fillMaxWidth() .verticalScroll(scroll), verticalArrangement = verticalArrangement, ) { - items.forEach { item -> - itemContent(item) + items.forEach { item -> + key(key?.invoke(item)) { + itemContent(item) + } } }Preview 추가 예시:
`@Preview`(showBackground = true) `@Composable` private fun AdaptiveSheetListPreview() { AdaptiveSheetList( items = listOf("Item 1", "Item 2", "Item 3"), threshold = 5, ) { item -> Text(text = item, modifier = Modifier.padding(16.dp)) } }core/design-system/src/main/java/com/twix/designsystem/components/button/AppButton.kt (1)
19-49: 공통 버튼 컴포넌트가 잘 구현되어 있습니다! 다만 비활성화 상태의 시각적 피드백이 필요합니다.
Surface를 사용하여 클릭 가능한 버튼을 구현한 것은 좋습니다. 하지만enabled = false일 때 시각적으로 버튼이 비활성화되었음을 사용자에게 알려주지 않습니다. 이는 접근성(Accessibility) 관점에서 개선이 필요합니다.또한, 디자인 시스템 컴포넌트이므로 다양한 상태(enabled/disabled, 다른 색상 조합)에 대한 Preview가 있으면 좋겠습니다.
♻️ 비활성화 상태 시각적 피드백 추가 제안
`@Composable` fun AppButton( modifier: Modifier = Modifier, text: String, textColor: Color = CommonColor.White, backgroundColor: Color = GrayColor.C500, enabled: Boolean = true, cornerRadius: Dp = 12.dp, border: BorderStroke? = null, onClick: () -> Unit = {}, ) { + val finalBackgroundColor = if (enabled) backgroundColor else backgroundColor.copy(alpha = 0.5f) + val finalTextColor = if (enabled) textColor else textColor.copy(alpha = 0.5f) + Surface( - color = backgroundColor, + color = finalBackgroundColor, shape = RoundedCornerShape(cornerRadius), border = border, onClick = onClick, enabled = enabled, modifier = modifier, ) { AppText( text = text, - color = textColor, + color = finalTextColor, style = AppTextStyle.T2, textAlign = TextAlign.Center, modifier = Modifier .fillMaxWidth() .padding(vertical = 14.dp), ) } }core/design-system/src/main/java/com/twix/designsystem/components/bottomsheet/model/CommonBottomSheetConfig.kt (1)
11-20: 잘 구성된 Config 클래스입니다. 몇 가지 개선점을 제안드립니다.
handleTopPadding과handleBottomPadding이 정의되어 있지만,CommonBottomSheet.kt의SheetHandle에서는 하드코딩된11.dp를 사용하고 있습니다. Config 값이 실제로 적용되도록 수정하거나, 사용하지 않는다면 제거하는 것이 좋겠습니다.
maxHeightFraction에 대한 유효성 검증이 없습니다. 음수나 1.0을 초과하는 값이 전달될 경우 예기치 않은 동작이 발생할 수 있습니다.♻️ maxHeightFraction 검증 추가 제안
`@Immutable` data class CommonBottomSheetConfig( val scrimColor: Color = DimmedColor.D070, val shape: Shape = RoundedCornerShape(topStart = 20.dp, topEnd = 20.dp), val showHandle: Boolean = true, val handleTopPadding: Dp = 11.dp, val handleBottomPadding: Dp = 11.dp, - val maxHeightFraction: Float = 0.8f, // 바텀시트가 최대로 커질 수 있는 높이를 비율로 설정 + val maxHeightFraction: Float = 0.8f, // 바텀시트가 최대로 커질 수 있는 높이를 비율로 설정 val dismissOnScrimClick: Boolean = true, -) +) { + init { + require(maxHeightFraction in 0.1f..1.0f) { + "maxHeightFraction must be between 0.1 and 1.0" + } + } +}core/design-system/src/main/java/com/twix/designsystem/components/bottomsheet/CommonBottomSheet.kt (4)
66-76: 애니메이션 타이밍 관련 개선 제안
delay(200)이 하드코딩되어 있는데, 이 값이BottomSheetContent의 exit 애니메이션 duration(180ms)과 정확히 일치하지 않습니다. 애니메이션이 완료되기 전에rendering이false가 되거나, 불필요하게 오래 기다릴 수 있습니다.애니메이션 duration을 상수로 추출하여 일관성을 유지하는 것이 좋겠습니다.
♻️ 애니메이션 상수 추출 제안
private const val EXIT_ANIMATION_DURATION = 180 // LaunchedEffect 내부에서 delay(EXIT_ANIMATION_DURATION.toLong())
191-204: Config의 handle padding 값이 적용되지 않고 있습니다.
CommonBottomSheetConfig에handleTopPadding과handleBottomPadding이 정의되어 있지만,SheetHandle에서는 하드코딩된11.dp를 사용하고 있습니다.♻️ Config 값 적용 제안
`@Composable` -private fun SheetHandle(modifier: Modifier = Modifier) { +private fun SheetHandle( + modifier: Modifier = Modifier, + topPadding: Dp, + bottomPadding: Dp, +) { Box( modifier = modifier - .padding(vertical = 11.dp) + .padding(top = topPadding, bottom = bottomPadding) .width(44.dp) .height(6.dp) .background( color = GrayColor.C100, shape = RoundedCornerShape(2.55.dp), ), ) }호출부에서:
if (config.showHandle) { SheetHandle( modifier = Modifier .align(Alignment.CenterHorizontally), + topPadding = config.handleTopPadding, + bottomPadding = config.handleBottomPadding, ) }
159-168: Dark Mode 지원이 고려되지 않았습니다.
Surface의color가CommonColor.White로 하드코딩되어 있어 Dark Mode에서 적절하지 않은 UI가 표시될 수 있습니다.코딩 가이드라인에 따르면 디자인 시스템 컴포넌트는 Dark Mode 지원을 고려해야 합니다.
MaterialTheme.colorScheme.surface를 사용하거나,CommonBottomSheetConfig에backgroundColor옵션을 추가하는 것을 권장합니다.♻️ Dark Mode 지원 제안
Surface( shape = config.shape, tonalElevation = 0.dp, shadowElevation = 16.dp, - color = CommonColor.White, + color = config.backgroundColor, // Config에 backgroundColor 추가 modifier = Modifier .fillMaxWidth() .heightIn(max = sheetMaxHeight), )As per coding guidelines: "[디자인 시스템 리뷰 가이드] - Dark Mode 지원 여부"
122-131: 접근성(Accessibility) 개선이 필요합니다.Scrim 영역에 접근성 정보가 없습니다. 스크린 리더 사용자가 바텀시트를 닫을 수 있도록 시맨틱 정보를 추가하는 것이 좋겠습니다.
♻️ 접근성 개선 제안
Box( modifier = Modifier .fillMaxSize() .background(config.scrimColor) + .semantics { + contentDescription = "바텀시트 닫기" + onClick(label = "닫기") { + onDismissRequest() + true + } + } .noRippleClickable( enabled = config.dismissOnScrimClick, onClick = onDismissRequest, ), )As per coding guidelines: "[디자인 시스템 리뷰 가이드] - 접근성(Accessibility) 고려 여부"
core/design-system/src/main/java/com/twix/designsystem/components/calendar/MonthGrid.kt (2)
54-60: 입력 labels 순서에 대한 문서화 개선 제안
rotateLabels함수는 입력labels가 일요일부터 시작하는 순서(일, 월, 화, ...)라고 가정하고 있습니다. 이 가정이 KDoc에 명시되어 있지 않아 사용자가 혼동할 수 있습니다.♻️ KDoc 개선 제안
/** * 월, 화, 수, 목, 금, 토, 일 label의 순서를 결정하는 메서드 + * `@param` labels 일요일부터 토요일 순서로 정렬된 요일 라벨 리스트 (크기: 7) * `@param` firstDayOfWeek 첫 번째 날짜의 요일 -> 기본값은 일요일 -> 일, 월, 화, 수, 목, 금, 토 순으로 반환 + * `@return` firstDayOfWeek부터 시작하는 순서로 회전된 라벨 리스트 * */
41-48: 코드는 수학적으로 올바르지만, 명확성을 위해 주석을 추가하는 것이 좋겠습니다.
DayOfWeek.value는 월요일=1부터 일요일=7까지의 값을 반환합니다.% 7연산은 이를 일요일=0부터 월요일=1까지의 0-6 범위로 정규화하는 의도적인 연산으로, 캘린더 오프셋 계산에 표준적으로 사용되는 방식입니다.예를 들어:
DayOfWeek.SUNDAY (7) % 7 = 0DayOfWeek.MONDAY (1) % 7 = 1현재
(d - s + 7) % 7공식은 음수 차이를 올바르게 처리하므로 로직 자체는 정확합니다. 다만 코드의 가독성을 위해 다음과 같이 명확한 주석을 추가하면 다른 개발자가 의도를 더 쉽게 파악할 수 있을 것 같습니다:fun daysFromStartOfWeek( day: DayOfWeek, start: DayOfWeek, ): Int { // DayOfWeek.value (1-7)를 0-6 범위로 정규화 val d = day.value % 7 // SUNDAY(7) → 0, MONDAY(1) → 1, ... val s = start.value % 7 return (d - s + 7) % 7 // 음수 처리하여 항상 0-6 범위의 양수 반환 }core/design-system/src/main/java/com/twix/designsystem/components/calendar/Calendar.kt (3)
132-135: 불필요한 재계산 발생 가능성
remember(headerDate, selectedDate)에서selectedDate를 key로 포함하고 있지만,buildMonthGrid는headerDate만 사용합니다.selectedDate가 변경될 때마다 그리드가 불필요하게 재계산됩니다.♻️ 수정 제안
val grid = - remember(headerDate, selectedDate) { + remember(headerDate) { buildMonthGrid(initialDate = headerDate) }
96-121: 네비게이션 버튼의 접근성 개선 제안이전/다음 달 버튼에
contentDescription은 있지만, 클릭 가능한 영역의 크기가 터치 타겟 최소 크기(48dp)보다 작을 수 있습니다. 또한noRippleClickable에 시맨틱 클릭 액션이 없습니다.♻️ 접근성 개선 제안
Image( painter = painterResource(R.drawable.ic_arrow_m_left), contentDescription = "previous month", modifier = Modifier - .padding(6.dp) - .size(24.dp) + .size(48.dp) // 최소 터치 타겟 크기 + .padding(12.dp) + .semantics { + role = Role.Button + } .noRippleClickable(onClick = onPreviousMonth), )As per coding guidelines: "[디자인 시스템 리뷰 가이드] - 접근성(Accessibility) 고려 여부"
38-81: Preview Composable이 누락되었습니다.
Calendar컴포넌트에 대한@Preview함수가 없어 개발 중 미리보기가 어렵습니다. 다른 컴포넌트들처럼 Preview를 추가하면 좋겠습니다.♻️ Preview 추가 제안
`@Preview`(showBackground = true) `@Composable` private fun CalendarPreview() { TwixTheme { Calendar( initialDate = LocalDate.now(), onComplete = {}, ) } }As per coding guidelines: "[Feature 모듈 - MVI 패턴 리뷰 가이드] - Preview Composable이 제공되는가?"
feature/main/src/main/java/com/twix/main/MainScreen.kt (1)
69-77: 탭 콘텐츠 플레이스홀더
MainTab.STATS와MainTab.COUPLE탭이 빈Box로 처리되어 있습니다. 향후 구현 예정이라면 TODO 주석을 추가하여 명시하는 것이 좋겠습니다.- MainTab.STATS -> Box(modifier = Modifier.fillMaxSize()) - MainTab.COUPLE -> Box(modifier = Modifier.fillMaxSize()) + MainTab.STATS -> Box(modifier = Modifier.fillMaxSize()) // TODO: StatsRoute 구현 + MainTab.COUPLE -> Box(modifier = Modifier.fillMaxSize()) // TODO: CoupleRoute 구현feature/main/src/main/java/com/twix/home/component/HomeTopBar.kt (1)
132-138: Preview 콜백 파라미터 네이밍 일관성Preview에서 콜백들이 위치 기반으로 전달되고 있어 가독성이 떨어집니다. Named parameter를 사용하면 더 명확해집니다.
♻️ 가독성 개선 제안
HomeTopBar( monthYearText = "1월 22일", - {}, - {}, - {}, - {}, + onNotificationClick = {}, + onSettingClick = {}, + onMoveToToday = {}, + onShowCalendarBottomSheet = {}, )
chanho0908
left a comment
chanho0908
left a comment
There was a problem hiding this comment.
고생했어 현수야 ! 기능 구현하느라 넘 바빠서 리뷰가 늦어서 미안해 🥲
코드 리뷰할 때 마다 감탄하면서 보는 것 같은데 ,,, 이게 현업핑인가 ? 🫣
배우는 입장이라 많은 내용을 남기진 못했지만 같이 고민해보면 좋을 것 같은 것들을 남겨봤어 !
| @@ -0,0 +1,300 @@ | |||
| package com.twix.designsystem.components.bottomsheet | |||
There was a problem hiding this comment.
바텀시트의 마지막 아이템이 하단과 완전 딱 붙어있어서 약간의 간격을 주는건 어떨까 ?
There was a problem hiding this comment.
이 부분은 디자인상으로 하단 패딩이 조금씩 달라서 content 넘길 때 패딩 넣는 걸로 설계했어요!
| @Composable | ||
| fun <T> AdaptiveSheetList( |
There was a problem hiding this comment.
아이템이 적을 땐 Column을, 많을 땐 LazyColumn를 사용하는 구조일까?
생각 못해본 구조인데 너무 좋은 것 같아 !
| verticalArrangement: Arrangement.Vertical = Arrangement.Top, | ||
| items: List<T>, | ||
| key: ((T) -> Any)? = null, | ||
| threshold: Int = 12, |
There was a problem hiding this comment.
단순 궁금증인데 12는 어떤 기준으로 정하게 된걸까 ?
There was a problem hiding this comment.
원래는 스크롤이 발생할 때쯤 LazyColumn으로 변경하려고 했는데, 개수를 정하기가 조금 애매한 거 같아서 넉넉하게 12로 잡았어요!
| var headerDate by remember { mutableStateOf(initialDate) } | ||
| // 달력의 선택된 날짜를 나타내는 상태 변수 | ||
| var selectedDate by remember { mutableStateOf(initialDate) } |
There was a problem hiding this comment.
구성 변경시 헤더의 날짜와 선택된 날짜가 초기화 되고 있어 !
remember 대신 rememberSavable 을 사용하는건 어떨까 ?
dd.mp4
default.mp4
| onNextMonth = { headerDate = headerDate.plusMonths(1) }, | ||
| onPreviousMonth = { headerDate = headerDate.minusMonths(1) }, |
There was a problem hiding this comment.
사실 큰 차이는 없다고 느껴질수도 있을 것 같지만 한가지 제안해보자면
도메인에 LocalDate를 담는 객체를 만들어서 NextMonth, PreviousMonth 같은 메서드를 만들어 날짜를 관리하는 책임을 부여하는건 어떨까 ?
There was a problem hiding this comment.
일단 지금은 한달씩 움직이는 게 전부라서 당장 도입하기보다는 나중에 날짜 조작하는 로직을 한번에 묶어서 개선해보면 좋을 거 같아요! 각자 지금 맡은 거 구현 다 끝내고 같이 얘기해봐요
이슈 번호
작업내용
결과물
2026-01-28.5.24.05.mov
리뷰어에게 추가로 요구하는 사항 (선택)
공통 컴포넌트를 몇개 구현했는데 기존 코드에서 개별적으로 구현한 게 있다면 수정하면 좋을 것 같아요!
사용법
CommonBottomSheet
등장, 퇴장 애니메이션이 구현되어 있고, 바텀시트에 대한 설정은 Config를 통해서 설정 가능합니다! 아직 Handle을 잡아서 크기 조절하는 것은 구현하지 않아서 참고해주세요!
이런식으로 바텀시트 내부 content만 구현해서 전달하면 되고, content에는 ColumnScope가 적용되어 있습니다!
AppButton
버튼 종류가 많아서 우선은 기본적인 흑/백 버튼 재사용할 수 있게 구현했습니다.
Calendar
캘린더는 초기 날짜를 넘기면 내부적으로 headerDate, selectedDate로 나눠서 관리하도록 구현했습니다!