[Feature] 콜밴 글쓰기 시간 선택 필드 추가#1325
[Feature] 콜밴 글쓰기 시간 선택 필드 추가#1325JaeYoung290 wants to merge 11 commits intofeature/#1283-callvan-create-date-fieldfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Walkthrough새로운 공개 Composable Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Field as CallvanTimeField
participant Picker as CallvanTimePickerCard
participant App as Callbacks
User->>Field: 필드 클릭 (onFieldClick)
Field->>Picker: 보여주기/숨기기 (isPickerVisible)
alt Picker visible
User->>Picker: AM/PM 선택 (onAmPmIndexChange)
User->>Picker: 시간 선택 (onHourIndexChange)
User->>Picker: 분 선택 (onMinuteIndexChange)
User->>Picker: Reset 클릭 (onReset)
User->>Picker: Confirm 클릭 (onConfirm)
Picker->>App: 콜백 호출(onAmPmIndexChange/onHourIndexChange/onMinuteIndexChange/onReset/onConfirm)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
JaeYoung290
force-pushed
the
feature/#1283-callvan-create-date-field
branch
from
a30bf04 to
e5a40df
Compare
JaeYoung290
force-pushed
the
feature/#1283-callvan-create-time-field
branch
from
73b4a7a to
766e48f
Compare
| amPmText: String, | ||
| formattedTime: String, | ||
| isPickerVisible: Boolean, | ||
| isAm: Boolean, | ||
| selectedHour: Int, | ||
| selectedMinute: Int, | ||
| onFieldClick: () -> Unit, | ||
| onAmPmIndexChange: (Int) -> Unit, | ||
| onHourIndexChange: (Int) -> Unit, | ||
| onMinuteIndexChange: (Int) -> Unit, | ||
| onReset: () -> Unit, | ||
| onConfirm: () -> Unit |
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .border(1.dp, KoinTheme.colors.neutral400, RoundedCornerShape(4.dp)) |
| style = KoinTheme.typography.regular14, | ||
| color = KoinTheme.colors.neutral800, |
| val amPmItems = listOf("AM", "PM") | ||
| val hourItems = (1..12).map { it.toString() } | ||
| val minuteItems = (0..59).map { "%02d".format(it) } | ||
|
|
||
| val amPmIndex = if (isAm) 0 else 1 | ||
| val hourIndex = if (selectedHour == 0) 11 else (selectedHour - 1).coerceIn(0, 11) | ||
| val minuteIndex = selectedMinute.coerceIn(0, 59) |
|
✅ CI 완료. CodeRabbit 리뷰를 요청합니다. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…1283-callvan-create-time-field
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt (1)
34-47: Public Composable에modifier파라미터 추가 권장Compose 컨벤션 및 코딩 가이드라인에 따르면, public Composable 함수는
modifier파라미터를 받아 외부에서 레이아웃 조정이 가능하도록 해야 합니다. 필수 파라미터 뒤, 기본값이 있는 파라미터 앞에 배치하세요.♻️ 수정 제안
`@Composable` fun CallvanTimeField( amPmText: String, formattedTime: String, isPickerVisible: Boolean, isAm: Boolean, selectedHour: Int, selectedMinute: Int, + modifier: Modifier = Modifier, onFieldClick: () -> Unit = {}, onAmPmIndexChange: (Int) -> Unit = {}, onHourIndexChange: (Int) -> Unit = {}, onMinuteIndexChange: (Int) -> Unit = {}, onReset: () -> Unit = {}, onConfirm: () -> Unit = {} ) { - Column(modifier = Modifier.fillMaxWidth()) { + Column(modifier = modifier.fillMaxWidth()) {As per coding guidelines: "@composable 함수의 파라미터는 기본값이 없는 필수 파라미터, modifier, 기본값이 있는 파라미터 순서로 선언"
🤖 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/create/component/CallvanTimeField.kt` around lines 34 - 47, CallvanTimeField의 시그니처에 public Composable 관례를 따라 modifier: Modifier = Modifier 파라미터를 필수 파라미터들(예: amPmText, formattedTime, isPickerVisible, isAm, selectedHour, selectedMinute) 뒤, 기본값 있는 콜백들 앞에 추가하고(즉 CallvanTimeField(..., selectedMinute: Int, modifier: Modifier = Modifier, onFieldClick: () -> Unit = {}, ...)), 내부에서는 루트 레이아웃(예: 최상위 Box/Column/Surface)에 이 modifier를 전달하여 외부에서 레이아웃 조정이 가능하도록 하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt`:
- Around line 146-147: Remove the unnecessary trailing comma before the closing
parenthesis on the Modifier.weight call in the CallvanTimeField composable;
locate the line containing "modifier = Modifier.weight(1f)," and change it to
remove the comma so it ends with the closing parenthesis, ensuring KtLint no
longer flags a trailing-comma error.
---
Nitpick comments:
In
`@feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt`:
- Around line 34-47: CallvanTimeField의 시그니처에 public Composable 관례를 따라 modifier:
Modifier = Modifier 파라미터를 필수 파라미터들(예: amPmText, formattedTime, isPickerVisible,
isAm, selectedHour, selectedMinute) 뒤, 기본값 있는 콜백들 앞에 추가하고(즉
CallvanTimeField(..., selectedMinute: Int, modifier: Modifier = Modifier,
onFieldClick: () -> Unit = {}, ...)), 내부에서는 루트 레이아웃(예: 최상위 Box/Column/Surface)에
이 modifier를 전달하여 외부에서 레이아웃 조정이 가능하도록 하세요.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 885b3fc5-01a9-4a4d-831d-e3a50a8fec25
📒 Files selected for processing (1)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| amPmText: String, | ||
| formattedTime: String, | ||
| isPickerVisible: Boolean, | ||
| isAm: Boolean, |
There was a problem hiding this comment.
[Major] amPmText와 isAm의 이중 진실 공급원(Dual Source of Truth) 문제
amPmText: String과 isAm: Boolean은 동일한 AM/PM 상태를 두 가지 별개의 파라미터로 표현하고 있습니다.
amPmText는 필드 텍스트 표시에 사용 ("오전","오후")isAm은 픽커 내부의 선택 인덱스 계산에 사용
호출 측에서 amPmText = "오전"이지만 isAm = false를 전달하면 필드는 "오전"을 표시하면서 픽커는 "PM"이 선택된 상태로 열리는 불일치가 발생합니다.
amPmText를 제거하고 isAm에서 파생시키거나, 반대로 isAm을 제거하고 amPmText를 비교하는 방식으로 단일 진실 공급원을 유지하는 것을 권장합니다.
| amPmText: String, | |
| formattedTime: String, | |
| isPickerVisible: Boolean, | |
| isAm: Boolean, | |
| isAm: Boolean, | |
| formattedTime: String, | |
| isPickerVisible: Boolean, |
그리고 필드의 AM/PM 텍스트는 컴포넌트 내부에서 파생하는 방식으로 변경:
val amPmText = if (isAm) stringResource(R.string.callvan_am) else stringResource(R.string.callvan_pm)
| onReset: () -> Unit, | ||
| onConfirm: () -> Unit | ||
| ) { | ||
| val amPmItems = remember { listOf("AM", "PM").toImmutableList() } |
There was a problem hiding this comment.
[Minor] AM/PM 픽커 아이템이 영어 하드코딩됨
필드에 표시되는 amPmText는 부모로부터 "오전"/"오후"와 같은 현지화된 문자열을 받지만, 픽커 내부의 amPmItems는 "AM", "PM"으로 하드코딩되어 있습니다. 동일 컴포넌트 안에서 언어가 혼용되는 UX 불일치가 발생합니다.
| val amPmItems = remember { listOf("AM", "PM").toImmutableList() } | |
| val amPmItems = remember { listOf("오전", "오후").toImmutableList() } |
또는 string resource를 사용하는 방식이 더 이상적입니다:
val amLabel = stringResource(R.string.callvan_am)
val pmLabel = stringResource(R.string.callvan_pm)
val amPmItems = remember(amLabel, pmLabel) { listOf(amLabel, pmLabel).toImmutableList() }
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanPickerFooter.kt (2)
16-38: 재사용 가능한 컴포저블에 Modifier 파라미터 추가 권장Compose 컴포넌트의 재사용성과 유연성을 위해
Modifier파라미터를 추가하는 것이 좋습니다. 호출 측에서 패딩이나 정렬 등을 조정할 수 있게 됩니다.♻️ Modifier 파라미터 추가 제안
`@Composable` fun CallvanPickerFooter( onReset: () -> Unit, - onConfirm: () -> Unit + onConfirm: () -> Unit, + modifier: Modifier = Modifier ) { Row( - modifier = Modifier.fillMaxWidth(), + modifier = modifier.fillMaxWidth(), horizontalArrangement = Arrangement.spacedBy(16.dp, Alignment.End) ) {🤖 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/create/component/CallvanPickerFooter.kt` around lines 16 - 38, Add a Modifier parameter to CallvanPickerFooter so callers can customize layout; update the function signature to accept a modifier: Modifier = Modifier and apply it to the root Row (replace Modifier.fillMaxWidth() with modifier.fillMaxWidth()), leaving existing onReset/onConfirm behavior unchanged and preserving default Modifier value for backward compatibility.
25-36: 클릭 가능한 Text에 시맨틱 및 ripple 효과 고려현재
clickable수식어는 기본 ripple 효과를 제공하지만, 버튼 역할을 명시적으로 나타내기 위해role = Role.Button을 추가하면 접근성이 향상됩니다. 또한 최소 터치 영역(48dp)을 확보하기 위해 패딩 추가를 고려해 보세요.♻️ 접근성 개선 제안
+import androidx.compose.ui.semantics.Role + Text( text = stringResource(R.string.callvan_create_picker_reset), style = RebrandKoinTheme.typography.medium14, color = RebrandKoinTheme.colors.primary500, - modifier = Modifier.clickable(onClick = onReset) + modifier = Modifier + .clickable(role = Role.Button, onClick = onReset) + .padding(vertical = 8.dp) )🤖 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/create/component/CallvanPickerFooter.kt` around lines 25 - 36, 두 Text 클릭 컴포저블(확인 onConfirm, 초기화 onReset)에 접근성 및 터치영역을 개선하세요: Modifier.clickable에 role = Role.Button을 추가해 버튼 역할을 시맨틱으로 노출하고, 최소 터치 영역을 확보하기 위해 해당 Modifier 체인에 패딩 또는 최소 인터랙티브 사이즈(예: Modifier.padding(8.dp) 또는 Modifier.minimumInteractiveComponentSize()/sizeIn(minWidth = 48.dp, minHeight = 48.dp))를 적용하여 터치 대상 크기를 늘리세요; 관련 심볼: onReset, onConfirm, Modifier.clickable, Role.Button, Text 컴포저블.
🤖 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/create/component/CallvanPickerFooter.kt`:
- Around line 16-38: Add a Modifier parameter to CallvanPickerFooter so callers
can customize layout; update the function signature to accept a modifier:
Modifier = Modifier and apply it to the root Row (replace
Modifier.fillMaxWidth() with modifier.fillMaxWidth()), leaving existing
onReset/onConfirm behavior unchanged and preserving default Modifier value for
backward compatibility.
- Around line 25-36: 두 Text 클릭 컴포저블(확인 onConfirm, 초기화 onReset)에 접근성 및 터치영역을
개선하세요: Modifier.clickable에 role = Role.Button을 추가해 버튼 역할을 시맨틱으로 노출하고, 최소 터치 영역을
확보하기 위해 해당 Modifier 체인에 패딩 또는 최소 인터랙티브 사이즈(예: Modifier.padding(8.dp) 또는
Modifier.minimumInteractiveComponentSize()/sizeIn(minWidth = 48.dp, minHeight =
48.dp))를 적용하여 터치 대상 크기를 늘리세요; 관련 심볼: onReset, onConfirm, Modifier.clickable,
Role.Button, Text 컴포저블.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be41faf1-d75a-4ae4-beb7-7541750f3ac2
📒 Files selected for processing (4)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanPickerFooter.ktfeature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.ktfeature/callvan/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/callvan/src/main/res/values/strings.xml
- feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
There was a problem hiding this comment.
b084470 수정사항 확인 완료 — 변경 내용 검증 결과:
- amPmText 파라미터 제거 ✅ —
amPmText를 외부에서 주입받지 않고isAm+stringResource로 내부에서 파생하도록 수정되어 API가 더 일관됩니다. - AM/PM 항목 현지화 ✅ — 피커 항목이 하드코딩된 "AM"/"PM"에서
callvan_am(오전) /callvan_pm(오후) 문자열 리소스로 교체되었습니다. - hourIndex 계산 단순화 ✅ —
if (selectedHour == 0) 11 else ...엣지 케이스 제거.selectedHour가 1–12 범위로 보장되므로 0은 유효하지 않은 값이며,(selectedHour - 1).coerceIn(0, 11)이 올바른 0-based 인덱스를 생성합니다. - Locale.ROOT 적용 ✅ — 이후 커밋에서 minuteItems에
String.format(Locale.ROOT, ...)가 적용되었습니다.
...n/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanPickerFooter.kt
Show resolved
Hide resolved
| onReset: () -> Unit, | ||
| onConfirm: () -> Unit | ||
| ) { | ||
| Row( |
There was a problem hiding this comment.
[Minor] Row 자체에도 수직 패딩 부재
Row에 수직 패딩이 없어, Card 내부에서 HorizontalDivider 바로 아래에 버튼들이 붙어 보일 수 있습니다. 시각적 여백을 위해 Row에 패딩을 추가하는 것을 권장합니다.
| Row( | |
| Row( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .padding(vertical = 4.dp), | |
| horizontalArrangement = Arrangement.spacedBy(16.dp, Alignment.End) | |
| ) { |
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
Show resolved
Hide resolved
| Text( | ||
| text = formattedTime, |
There was a problem hiding this comment.
[Minor] 빈 시간 문자열에 대한 placeholder 미처리
formattedTime이 빈 문자열일 경우 Text가 비어 보입니다. 날짜 선택 필드(CallvanDateField)와 일관성을 맞추기 위해 hint 텍스트를 표시하는 것을 권장합니다.
| Text( | |
| text = formattedTime, | |
| Text( | |
| text = formattedTime.ifEmpty { stringResource(R.string.callvan_create_time_hint) }, | |
| style = RebrandKoinTheme.typography.regular14, | |
| color = if (formattedTime.isEmpty()) RebrandKoinTheme.colors.neutral400 | |
| else RebrandKoinTheme.colors.neutral800, | |
| modifier = Modifier | |
| .weight(1f) | |
| .padding(horizontal = 16.dp, vertical = 8.dp) | |
| ) |
| listOf(amLabel, pmLabel).toImmutableList() | ||
| } | ||
| val hourItems = remember { (1..12).map { it.toString() }.toImmutableList() } | ||
| val minuteItems = remember { (0..59).map { String.format(Locale.ROOT, "%02d", it) }.toImmutableList() } |
There was a problem hiding this comment.
[Trivial] 라인 길이 초과
이 줄은 120자를 초과합니다. 가독성을 위해 분리해 주세요.
| val minuteItems = remember { (0..59).map { String.format(Locale.ROOT, "%02d", it) }.toImmutableList() } | |
| val minuteItems = remember { | |
| (0..59).map { String.format(Locale.ROOT, "%02d", it) }.toImmutableList() | |
| } |
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanTimeField.kt
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
3 participants
PR 개요
이슈 번호: #1283
PR 체크리스트
작업사항
작업사항의 상세한 설명
출발 시각을 선택할 수 있는 필드와 시간 선택 카드를 추가합니다.
논의 사항
스크린샷
추가내용
Summary by CodeRabbit
새로운 기능
리팩터