[Feature] 콜밴 리스트 BottomSheet 및 FAB ui 구현#1366
[Feature] 콜밴 리스트 BottomSheet 및 FAB ui 구현#1366TTRR1007 wants to merge 5 commits intofeature/#1283-callvan-list-itemfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughCallVan 기능에 대한 새로운 UI 컴포넌트 및 관련 리소스를 추가합니다. 상태 매핑을 위한 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/CallvanFAB.kt (1)
32-36: Composable 파라미터 순서를 가이드에 맞춰 정렬해 주세요.Line 32-36은
modifier가 필수 파라미터(onClick)보다 앞에 있어, 팀 규칙(필수 → modifier → 기본값)과 다릅니다.As per coding guidelines "@composable 함수의 파라미터는 기본값이 없는 필수 파라미터, modifier, 기본값이 있는 파라미터 순서로 선언".♻️ 제안 패치
fun CallvanFAB( - modifier: Modifier = Modifier, onClick: () -> Unit, + modifier: Modifier = Modifier, windowInsets: WindowInsets = CallvanFABDefaults.windowInsets ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/CallvanFAB.kt` around lines 32 - 36, The CallvanFAB composable declares parameters in the wrong order: place required non-default parameters first, then modifier, then parameters with defaults; reorder the signature of CallvanFAB so onClick appears before modifier and windowInsets remains after modifier (keeping windowInsets' default CallvanFABDefaults.windowInsets) to match the project's Composable parameter guideline.feature/callvan/src/main/res/values/strings.xml (1)
110-110: 리소스 키 네이밍을 도메인 prefix로 통일하면 더 좋겠습니다.Line 110의
write_btn은 같은 블록의callvan_*패턴과 달라 검색성/충돌 회피 측면에서 불리합니다 (callvan_write_btn같은 형태 권장).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/callvan/src/main/res/values/strings.xml` at line 110, The resource key "write_btn" should be renamed to follow the domain prefix pattern (e.g., "callvan_write_btn") to match other keys and avoid collisions; update the <string name="write_btn"> entry to <string name="callvan_write_btn"> and then find and replace all usages of "write_btn" across layouts, Kotlin/Java files, and any view binding or getString calls (update imports/refs and rebuild to refresh R references), also remove or deprecate the old key if present elsewhere to avoid duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/CallvanFAB.kt`:
- Around line 32-36: The CallvanFAB composable declares parameters in the wrong
order: place required non-default parameters first, then modifier, then
parameters with defaults; reorder the signature of CallvanFAB so onClick appears
before modifier and windowInsets remains after modifier (keeping windowInsets'
default CallvanFABDefaults.windowInsets) to match the project's Composable
parameter guideline.
In `@feature/callvan/src/main/res/values/strings.xml`:
- Line 110: The resource key "write_btn" should be renamed to follow the domain
prefix pattern (e.g., "callvan_write_btn") to match other keys and avoid
collisions; update the <string name="write_btn"> entry to <string
name="callvan_write_btn"> and then find and replace all usages of "write_btn"
across layouts, Kotlin/Java files, and any view binding or getString calls
(update imports/refs and rebuild to refresh R references), also remove or
deprecate the old key if present elsewhere to avoid duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ae136d8-0011-4fed-9835-df900f19fc85
📒 Files selected for processing (8)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/enums/ConfirmType.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/CallvanFAB.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/CompleteBottomSheet.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/ConfirmBottomSheet.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/component/LoginBottomSheet.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/list/model/CallvanFABDefaults.ktfeature/callvan/src/main/res/drawable/ic_write_fab.xmlfeature/callvan/src/main/res/values/strings.xml
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .border( | ||
| width = 1.dp, | ||
| color = RebrandKoinTheme.colors.primary400, | ||
| shape = RoundedCornerShape(50) | ||
| ) | ||
| .background( | ||
| color = KoinTheme.colors.neutral50, | ||
| shape = RoundedCornerShape(50) | ||
| ) | ||
| .padding(vertical = 8.dp, horizontal = 12.dp) | ||
| .noRippleClickable(onClick = onClick) | ||
| ) { |
There was a problem hiding this comment.
[Major] noRippleClickable 수정자 순서 오류 — 클릭 영역이 padding 이후로 축소됩니다
Compose에서 수정자는 체인 순서대로 적용되기 때문에, noRippleClickable이 .padding() 뒤에 오면 터치 인식 영역이 padding을 제외한 내부 영역으로 축소됩니다. FAB 특성상 padding 영역을 포함한 전체 영역이 터치 대상이어야 합니다.
noRippleClickable을 padding 앞으로 이동해야 합니다:
| .border( | |
| width = 1.dp, | |
| color = RebrandKoinTheme.colors.primary400, | |
| shape = RoundedCornerShape(50) | |
| ) | |
| .background( | |
| color = KoinTheme.colors.neutral50, | |
| shape = RoundedCornerShape(50) | |
| ) | |
| .padding(vertical = 8.dp, horizontal = 12.dp) | |
| .noRippleClickable(onClick = onClick) | |
| ) { | |
| modifier = Modifier | |
| .border( | |
| width = 1.dp, | |
| color = RebrandKoinTheme.colors.primary400, | |
| shape = RoundedCornerShape(50) | |
| ) | |
| .background( | |
| color = KoinTheme.colors.neutral50, | |
| shape = RoundedCornerShape(50) | |
| ) | |
| .noRippleClickable(onClick = onClick) | |
| .padding(vertical = 8.dp, horizontal = 12.dp) |
| import `in`.koreatech.koin.core.designsystem.noRippleClickable | ||
| import `in`.koreatech.koin.core.designsystem.theme.KoinTheme | ||
| import `in`.koreatech.koin.core.designsystem.theme.RebrandKoinTheme |
There was a problem hiding this comment.
[Minor] 두 가지 테마 시스템 혼용
KoinTheme과 RebrandKoinTheme을 같은 컴포넌트 내에서 함께 사용하고 있습니다. 리브랜딩 마이그레이션이 진행 중이라면 불가피할 수 있지만, 최소한 주석으로 의도를 명시하거나 동일한 값에 대해 어느 테마를 사용할지 일관성을 유지하는 것이 좋습니다.
또한 아래 **Preview(line 73~80)**에서는 KoinTheme만 사용하고 있어 RebrandKoinTheme.colors.primary600이 적용된 텍스트 색상이 Preview에 올바르게 반영되지 않을 수 있습니다. Preview에서도 RebrandKoinTheme { KoinTheme { ... } } 형태로 두 테마를 모두 래핑하거나, 프로젝트 내 공통 Preview 래퍼를 사용하는 것을 권장합니다.
| @Composable | ||
| private fun LoginBottomSheetPreview() { | ||
| CallvanBottomSheet( | ||
| title = "콜밴팟에 참여하려면 로그인이 필요해요.", | ||
| onDismiss = {}, | ||
| showCloseButton = false | ||
| ) { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(start = 32.dp, end = 32.dp, top = 16.dp, bottom = 12.dp), | ||
| verticalArrangement = Arrangement.spacedBy(16.dp) | ||
| ) { | ||
| Button( | ||
| onClick = {}, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = KoinTheme.shapes.small, | ||
| contentPadding = PaddingValues(horizontal = 16.dp, vertical = 10.dp), | ||
| colors = ButtonDefaults.buttonColors( | ||
| containerColor = RebrandKoinTheme.colors.primary500 | ||
| ) | ||
| ) { | ||
| Text(text = "로그인하기", style = KoinTheme.typography.medium16) | ||
| } | ||
| OutlinedButton( | ||
| onClick = {}, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = KoinTheme.shapes.small, | ||
| contentPadding = PaddingValues(horizontal = 16.dp, vertical = 10.dp), | ||
| border = BorderStroke(1.dp, KoinTheme.colors.neutral300), | ||
| colors = ButtonDefaults.outlinedButtonColors( | ||
| contentColor = KoinTheme.colors.neutral600 | ||
| ) | ||
| ) { | ||
| Text(text = "닫기", style = KoinTheme.typography.medium16) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[Major] Preview가 실제 컴포넌트를 호출하지 않고 레이아웃 코드를 전부 중복합니다
현재 Preview는 LoginBottomSheet를 직접 호출하는 대신 내부 구현을 그대로 복붙하고 있습니다. 이렇게 되면:
- 실제 컴포넌트가 변경되어도 Preview가 업데이트되지 않아 유지보수 부채가 쌓입니다.
- Preview의 목적(컴포넌트의 시각적 동작 확인)을 달성하지 못합니다.
아래처럼 간단하게 수정하세요:
| @Composable | |
| private fun LoginBottomSheetPreview() { | |
| CallvanBottomSheet( | |
| title = "콜밴팟에 참여하려면 로그인이 필요해요.", | |
| onDismiss = {}, | |
| showCloseButton = false | |
| ) { | |
| Column( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .padding(start = 32.dp, end = 32.dp, top = 16.dp, bottom = 12.dp), | |
| verticalArrangement = Arrangement.spacedBy(16.dp) | |
| ) { | |
| Button( | |
| onClick = {}, | |
| modifier = Modifier.fillMaxWidth(), | |
| shape = KoinTheme.shapes.small, | |
| contentPadding = PaddingValues(horizontal = 16.dp, vertical = 10.dp), | |
| colors = ButtonDefaults.buttonColors( | |
| containerColor = RebrandKoinTheme.colors.primary500 | |
| ) | |
| ) { | |
| Text(text = "로그인하기", style = KoinTheme.typography.medium16) | |
| } | |
| OutlinedButton( | |
| onClick = {}, | |
| modifier = Modifier.fillMaxWidth(), | |
| shape = KoinTheme.shapes.small, | |
| contentPadding = PaddingValues(horizontal = 16.dp, vertical = 10.dp), | |
| border = BorderStroke(1.dp, KoinTheme.colors.neutral300), | |
| colors = ButtonDefaults.outlinedButtonColors( | |
| contentColor = KoinTheme.colors.neutral600 | |
| ) | |
| ) { | |
| Text(text = "닫기", style = KoinTheme.typography.medium16) | |
| } | |
| } | |
| } | |
| } | |
| @Preview | |
| @Composable | |
| private fun LoginBottomSheetPreview() { | |
| RebrandKoinTheme { | |
| LoginBottomSheet(onLogin = {}, onDismiss = {}) | |
| } | |
| } |
| shape = KoinTheme.shapes.small, | ||
| contentPadding = PaddingValues(horizontal = 16.dp, vertical = 10.dp), | ||
|
|
||
| border = BorderStroke(1.dp, KoinTheme.colors.neutral300), |
There was a problem hiding this comment.
[Trivial] 불필요한 빈 줄
같은 역할을 하는 ConfirmBottomSheet.kt의 동일한 위치와 비교해 불필요한 빈 줄이 있습니다. 일관성을 위해 제거하세요.
| border = BorderStroke(1.dp, KoinTheme.colors.neutral300), | |
| contentPadding = PaddingValues(horizontal = 16.dp, vertical = 10.dp), | |
| border = BorderStroke(1.dp, KoinTheme.colors.neutral300), |
| @Preview | ||
| @Composable | ||
| private fun CompleteBottomSheetPreview() { | ||
| CallvanConfirmBottomSheet( | ||
| title = "이용 완료 상태로 변경할까요?", | ||
| description = "• 콜밴 이용(탑승, 정산)이 모두 완료된 뒤 눌러야 합니다.\n• 완료 시 대화 내역이 삭제되며, 되돌릴 수 없습니다.", | ||
| confirmText = "확인", | ||
| cancelText = "취소", | ||
| onConfirm = {}, | ||
| onDismiss = {} | ||
| ) | ||
| } |
There was a problem hiding this comment.
[Major] Preview가 실제 컴포넌트를 호출하지 않고 내부 구현을 중복합니다
LoginBottomSheet와 동일한 문제입니다. Preview에서 하드코딩된 문자열을 직접 사용하므로, strings.xml이 바뀌어도 Preview에 반영되지 않습니다.
| @Preview | |
| @Composable | |
| private fun CompleteBottomSheetPreview() { | |
| CallvanConfirmBottomSheet( | |
| title = "이용 완료 상태로 변경할까요?", | |
| description = "• 콜밴 이용(탑승, 정산)이 모두 완료된 뒤 눌러야 합니다.\n• 완료 시 대화 내역이 삭제되며, 되돌릴 수 없습니다.", | |
| confirmText = "확인", | |
| cancelText = "취소", | |
| onConfirm = {}, | |
| onDismiss = {} | |
| ) | |
| } | |
| @Preview | |
| @Composable | |
| private fun CompleteBottomSheetPreview() { | |
| CompleteBottomSheet(onConfirm = {}, onDismiss = {}) | |
| } |
| onDismiss: () -> Unit, | ||
| title: String = when (confirmType) { | ||
| ConfirmType.JOIN -> stringResource(R.string.callvan_confirm_join_title) | ||
| ConfirmType.CANCEL_JOIN -> stringResource(R.string.callvan_confirm_cancel_title) | ||
| ConfirmType.CLOSE -> stringResource(R.string.callvan_confirm_close_title) | ||
| ConfirmType.REOPEN -> stringResource(R.string.callvan_confirm_reopen_title) |
There was a problem hiding this comment.
[Minor] 기본 파라미터에서 stringResource() 호출 — 권장하지 않는 Compose 패턴
stringResource()는 컴포저블 함수이므로, 기본 파라미터 표현식에서 호출하는 것은 Compose 공식 가이드라인에서 권장하지 않습니다. 언뜻 동작하는 것처럼 보이지만, 내부 컴파일러 최적화 과정에서 예기치 않은 동작을 유발할 수 있습니다.
title을 외부에서 오버라이드할 필요가 없다면, 파라미터에서 제거하고 함수 본문에서 처리하는 것이 더 명확합니다:
| onDismiss: () -> Unit, | |
| title: String = when (confirmType) { | |
| ConfirmType.JOIN -> stringResource(R.string.callvan_confirm_join_title) | |
| ConfirmType.CANCEL_JOIN -> stringResource(R.string.callvan_confirm_cancel_title) | |
| ConfirmType.CLOSE -> stringResource(R.string.callvan_confirm_close_title) | |
| ConfirmType.REOPEN -> stringResource(R.string.callvan_confirm_reopen_title) | |
| fun ConfirmBottomSheet( | |
| confirmType: ConfirmType, | |
| onConfirm: () -> Unit, | |
| onDismiss: () -> Unit | |
| ) { | |
| val (confirmType) { | |
| ConfirmType.JOIN -> stringResource(R.string.callvan_confirm_join_title) | |
| ConfirmType.CANCEL_JOIN -> stringResource(R.string.callvan_confirm_cancel_title) | |
| ConfirmType.CLOSE -> stringResource(R.string.callvan_confirm_close_title) | |
| ConfirmType.REOPEN -> stringResource(R.string.callvan_confirm_reopen_title) | |
| } |
외부 오버라이드 가능성을 열어두고 싶다면 title: String? = null로 받고 val resolvedTitle = title ?: when(confirmType) { ... } 패턴을 사용하세요.
| } | ||
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun ConfirmBottomSheetJoinPreview() { | ||
| RebrandKoinTheme { | ||
| ConfirmBottomSheet( | ||
| confirmType = ConfirmType.JOIN, | ||
| onConfirm = {}, | ||
| onDismiss = {} | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun ConfirmBottomSheetCancelJoinPreview() { | ||
| RebrandKoinTheme { | ||
| ConfirmBottomSheet( | ||
| confirmType = ConfirmType.CANCEL_JOIN, | ||
| onConfirm = {}, | ||
| onDismiss = {} | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun ConfirmBottomSheetClosePreview() { | ||
| RebrandKoinTheme { | ||
| ConfirmBottomSheet( | ||
| confirmType = ConfirmType.CLOSE, | ||
| onConfirm = {}, | ||
| onDismiss = {} | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun ConfirmBottomSheetReopenPreview() { | ||
| RebrandKoinTheme { | ||
| ConfirmBottomSheet( | ||
| confirmType = ConfirmType.REOPEN, | ||
| onConfirm = {}, | ||
| onDismiss = {} | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
[Good] 각 ConfirmType별 Preview를 개별적으로 작성한 점이 훌륭합니다 👍
4가지 상태를 모두 Preview로 커버한 덕분에 UI 회귀를 빠르게 발견할 수 있습니다. 이 패턴을 다른 Bottom Sheet에도 적용하면 좋겠습니다.
| object CallvanFABDefaults { | ||
| val windowInsets: WindowInsets | ||
| @Composable | ||
| get() = | ||
| WindowInsets.systemBars.only( | ||
| WindowInsetsSides.Horizontal + WindowInsetsSides.Bottom | ||
| ) |
There was a problem hiding this comment.
[Good] @Composable getter 패턴을 올바르게 사용하고 있습니다 👍
WindowInsets.systemBars는 컴포저블 컨텍스트에서만 접근 가능한데, @Composable 어노테이션을 getter에 달아 올바르게 처리했습니다. Material3의 FloatingActionButtonDefaults와 동일한 패턴입니다.
| <string name="callvan_confirm_negative">아니요</string> | ||
| <string name="callvan_complete_title">이용 완료 상태로 변경할까요?</string> | ||
| <string name="callvan_complete_description">• 콜밴 이용(탑승, 정산)이 모두 완료된 뒤 눌러야 합니다.\n• 완료 시 대화 내역이 삭제되며, 되돌릴 수 없습니다.</string> | ||
| <string name="write_btn">모집하기</string> |
There was a problem hiding this comment.
[Minor] 리소스 네이밍 컨벤션 불일치
이 파일의 모든 다른 문자열은 callvan_ 접두사를 사용하는데, write_btn만 예외입니다. 전역 네임스페이스 충돌 위험도 있습니다.
| <string name="write_btn">모집하기</string> | |
| <string name="callvan_fab_write_btn">모집하기</string> |
|
|
||
| companion object { | ||
| fun from(state: CallvanRouteState): ConfirmType? = when (state) { | ||
| CallvanRouteState.DEFAULT -> JOIN | ||
| CallvanRouteState.JOINED -> CANCEL_JOIN | ||
| CallvanRouteState.OWNER_ACTIVE -> CLOSE | ||
| CallvanRouteState.OWNER_CLOSED -> REOPEN | ||
| CallvanRouteState.CLOSED -> null | ||
| } | ||
| } |
There was a problem hiding this comment.
[Good] CLOSED 상태에 대해 null을 명시적으로 반환하는 설계가 좋습니다 👍
CallvanRouteState.CLOSED를 null로 매핑함으로써 호출부에서 Nullable 타입을 강제하여 "닫힌 상태에서는 ConfirmBottomSheet를 보여주지 않아야 한다"는 의도를 타입 시스템으로 표현했습니다.
다만 CallvanRouteState에 새로운 상태가 추가될 경우 when 블록이 컴파일 에러를 내어 누락을 즉시 알려주므로 안전합니다 (exhaustive when). 유지보수 관점에서 좋은 패턴입니다.
…lvan-list-item-bottomsheet
bcsd-b-bot
bot
added
ci:passed
and removed
ci:passed
coderabbit:complete
claude:complete
labels
|
@coderabbitai review |
…bottomsheet' into feature/#1283-callvan-list-item-bottomsheet # Conflicts: # feature/callvan/src/main/res/values/strings.xml
|
@coderabbitai review |
|
2 participants
PR 개요
이슈 번호:
PR 체크리스트
작업사항
작업사항의 상세한 설명
콜밴 리스트에서 나타나는 BottomSheet와 FAB (모집하기 버튼) ui를 구현했습니다
논의 사항
스크린샷
추가내용
Summary by CodeRabbit
릴리스 노트