Conversation
#47-goal-editor-screen
📝 WalkthroughWalkthrough새로운 goal-editor feature 모듈이 추가되었습니다. 목표 생성 및 편집 화면을 구현하는 GoalEditorScreen과 이를 관리하는 GoalEditorViewModel, Intent, SideEffect, UiState가 포함됩니다. Design System에 CommonSwitch, CommonDialog, UnderlineTextField 컴포넌트가 추가되었고, GoalIconType과 RepeatType enum이 domain에 추가되었습니다. 네비게이션 시스템에 GoalEditorGraph와 관련 routes가 통합되었으며, 기존의 HomeScreen과 MainScreen이 goal editor로 이동하는 콜백과 연결되었습니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 8
🤖 Fix all issues with AI agents
In
`@core/design-system/src/main/java/com/twix/designsystem/components/common/CommonSwitch.kt`:
- Around line 41-48: The Box in CommonSwitch.kt uses clickable which doesn't
expose switch semantics; replace clickable with Modifier.toggleable(value =
checked, onValueChange = { onClick(it) }, role = Role.Switch) (and import
androidx.compose.ui.semantics.Role) so TalkBack/VoiceOver announces "switch,
on/off"; ensure the existing size/clip/background/border modifiers remain and
remove the clickable call so the Composable (CommonSwitch) exposes proper
accessibility state via toggleable.
In
`@core/design-system/src/main/java/com/twix/designsystem/components/dialog/CommonDialog.kt`:
- Around line 113-116: The Surface in CommonDialog (around the Surface call
using shape = RoundedCornerShape(20.dp), color = CommonColor.White, and any
usages of GrayColor.C500) hardcodes light colors and breaks Dark Mode; update
the component to use semantic Material 3 colors
(MaterialTheme.colorScheme.surface / onSurface / primary / onPrimary as
appropriate) or the project's TwixTheme CompositionLocals instead of
CommonColor.White and GrayColor.C500 so colors respond to light/dark themes, and
ensure any text/icon colors inside CommonDialog also use semantic colorScheme
values rather than hardcoded colors.
- Around line 36-71: The CommonDialog currently lacks accessibility semantics:
update the dialog surface (inside DialogContent or wherever the dialog
Root/Surface is rendered) to include Modifier.semantics { role = Role.Dialog;
contentDescription = /* meaningful description or title */ } and add semantic
labels for action buttons provided via confirmText/dismissText (e.g., accessible
text or contentDescription for buttons in DialogContent). Also wire a
FocusRequester in CommonDialog (create and remember it, pass to DialogContent)
and requestFocus() when visible becomes true so screen readers and keyboard
focus move into the dialog; keep BackHandler(onDismissRequest) as-is. Locate and
modify the CommonDialog, DialogContent and button renderings to apply these
changes.
In
`@core/design-system/src/main/java/com/twix/designsystem/components/text_field/UnderlineTextField.kt`:
- Around line 39-42: The passed modifier parameter is being applied to internal
children instead of the root Column; move the incoming modifier to the outermost
Column composable (the Column in UnderlineTextField) so callers can control
layout, and replace usages of that parameter inside the internal Box and
HorizontalDivider with the default Modifier (i.e., use Modifier for those
children); update any references in the UnderlineTextField function signature
and usages to ensure modifier is applied only to the root Column.
In `@core/design-system/src/main/res/drawable/ic_plus.xml`:
- Around line 2-10: ic_plus.xml 하드코딩된 strokeColor("#171717") 때문에
GoalEditorScreen.kt에서 ColorFilter.tint(CommonColor.White)을 적용해도 의도대로 색이 바뀌지 않을 수
있으니 아이콘 색상을 리소스로 추출하고 다크/라이트 변형을 제공하도록 수정하세요: ic_plus.xml과 ic_minus.xml의
android:strokeColor 값을 직접 색상 코드 대신 `@color/icon_primary로` 바꾸고
res/values/colors.xml 및 res/values-night/colors.xml에 각각 icon_primary를 `#171717와`
`#FFFFFF로` 정의한 뒤 GoalEditorScreen.kt에서 기존 ColorFilter.tint(CommonColor.White) 사용이
필요 없다면 제거하거나 리소스 기반 색상 로딩으로 통일하세요.
In `@feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorScreen.kt`:
- Around line 309-312: When switching RepeatType (e.g., in the noRippleClickable
handler that sets internalSelectedRepeatType = RepeatType.WEEKLY), don't reset
internalRepeatCount to 0; set it to 1 so the repeat count has a sensible
minimum. Update all similar handlers (the other case at the other occurrence
around the block that currently sets internalRepeatCount = 0) to assign 1
instead, and ensure any UI/state that treats 0 as "unset" is adjusted or
clarified if 0 must remain meaningful.
- Around line 112-116: The internalSelectedIcon state in GoalEditorScreen is
only initialized from uiState.selectedIcon and won't update when
uiState.selectedIcon changes; change the remember usage to include
uiState.selectedIcon as a key (e.g., remember(uiState.selectedIcon) {
mutableStateOf(uiState.selectedIcon) }) so internalSelectedIcon is
re-created/synced when the external uiState.selectedIcon updates, ensuring
correct synchronization when entering edit mode or when the selected icon is
loaded/changed.
In
`@feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt`:
- Around line 64-69: The code block in GoalEditorViewModel uses a hardcoded
Korean toast string and an unnecessary coroutine launch; change the side-effect
to carry a message key (e.g., an enum or sealed class like
GoalEditorSideEffect.ToastMessageKey or a ToastType payload) instead of the raw
string so the UI layer resolves the string resource, and remove the
viewModelScope.launch by calling emitSideEffect(...) directly (since
handleIntent and emitSideEffect are suspend), i.e., when
currentState.endDateEnabled &&
currentState.endDate.isBefore(currentState.startDate) return by emitting a
ToastMessageKey via
emitSideEffect(GoalEditorSideEffect.ShowToast(ToastMessageKey.EndBeforeStart,
ToastType.ERROR)) without wrapping in viewModelScope.launch.
🧹 Nitpick comments (31)
core/design-system/src/main/java/com/twix/designsystem/components/bottomsheet/CommonBottomSheet.kt (3)
159-168: 다크 모드 지원을 위해 하드코딩된 색상 대신 테마 색상 사용을 고려해 주세요.현재
Surface의color가CommonColor.White로 하드코딩되어 있어 다크 모드에서 적절히 대응하지 못합니다. 디자인 시스템 컴포넌트는 다양한 테마 환경에서 재사용되므로, 테마 기반 색상을 사용하는 것이 권장됩니다.Material Design 3에서는 Bottom Sheet에
surfaceContainerLow또는surface색상을 권장합니다. 만약 프로젝트에서 커스텀 테마 시스템을 사용 중이라면, 해당 시스템에서 라이트/다크 모드에 따라 변경되는 색상을 사용하는 것이 좋습니다.♻️ 제안하는 수정 방향
Surface( shape = config.shape, tonalElevation = 0.dp, shadowElevation = 16.dp, - color = CommonColor.White, + color = MaterialTheme.colorScheme.surface, // 또는 프로젝트 테마 색상 modifier = Modifier .fillMaxWidth() .heightIn(max = sheetMaxHeight), )또는
CommonBottomSheetConfig에sheetColor파라미터를 추가하여 외부에서 제어할 수 있도록 하는 방법도 있습니다.As per coding guidelines:
core/design-system/**- Dark Mode 지원 여부를 검토해야 합니다.
191-204: 접근성 개선을 위해 SheetHandle에 시맨틱 정보 추가를 고려해 보세요.현재
SheetHandle은 시각적 요소만 있고 접근성 정보가 없습니다. 스크린 리더 사용자에게 이 요소가 드래그 핸들임을 알려주거나, 순수 장식용이라면Modifier.semantics { invisibleToUser() }를 통해 스크린 리더에서 건너뛰도록 설정할 수 있습니다.♻️ 접근성 개선 예시
`@Composable` private fun SheetHandle(modifier: Modifier = Modifier) { Box( modifier = modifier .padding(vertical = 11.dp) .width(44.dp) .height(6.dp) + .semantics { invisibleToUser() } // 장식용 요소로 표시 .background( color = GrayColor.C100, shape = RoundedCornerShape(2.55.dp), ), ) }또는 드래그 기능이 있다면:
.semantics { contentDescription = "드래그하여 시트 닫기" }As per coding guidelines:
core/design-system/**- 접근성(Accessibility) 고려 여부를 검토해야 합니다.
71-75: 애니메이션 지속 시간을 상수로 관리하면 유지보수성이 향상됩니다.
delay(200)이 하드코딩되어 있는데, 이 값은 exit 애니메이션 시간(slideOut: 180ms, fadeOut: 120ms)과 관련이 있습니다. 현재 200ms는 애니메이션이 완료되기에 충분하지만, 나중에 애니메이션 시간을 변경할 때 이 delay도 함께 수정해야 한다는 것을 놓치기 쉽습니다.애니메이션 관련 상수들을 companion object나 파일 상단에 정의하면 일관성을 유지하기 쉬워집니다.
♻️ 상수 추출 예시
private object BottomSheetAnimationDefaults { const val SLIDE_IN_DURATION = 220 const val SLIDE_OUT_DURATION = 180 const val FADE_IN_DURATION = 140 const val FADE_OUT_DURATION = 120 const val SCRIM_FADE_DURATION = 160 const val DISMISS_DELAY = 200L // SLIDE_OUT_DURATION 이상이어야 함 }core/design-system/src/main/java/com/twix/designsystem/components/common/CommonSwitch.kt (3)
46-46: Dark Mode 지원이 누락되었습니다.현재 색상이
CommonColor.White,GrayColor.C500으로 하드코딩되어 있어 다크 모드에서도 동일한 색상이 적용됩니다. 다크 모드에서 흰색 배경의 스위치는 시각적으로 튀거나 가독성 문제가 발생할 수 있습니다.테마 시스템에서 다크/라이트 모드에 따라 적응하는 색상을 사용하거나, 별도의 다크 모드 색상 세트를 정의하는 것을 권장합니다. 프로젝트의 다른 컴포넌트들이 다크 모드를 어떻게 처리하는지 확인해 주시겠어요?
As per coding guidelines, 디자인 시스템 리뷰 가이드에서 Dark Mode 지원 여부를 검토해야 합니다.
Also applies to: 58-58
33-33: 불필요한 연산을 단순화할 수 있습니다.
0.dp.toPx()는 항상0f를 반환합니다. density 변환이 필요하지 않으므로 직접0f를 사용하는 것이 더 명확합니다.✨ 단순화 제안
- val minBound = with(density) { 0.dp.toPx() } + val minBound = 0f
37-37: 애니메이션 지속 시간이 다소 길어 보입니다.500ms는 스위치 토글 애니메이션으로는 비교적 긴 편입니다. 일반적으로 스위치 애니메이션은 150-300ms 정도가 즉각적인 피드백을 제공하면서도 부드러운 전환을 보여줍니다. Material Design에서는 약 200ms를 권장합니다.
의도적으로 느린 애니메이션을 선택하신 것인지 확인 부탁드립니다. 사용자 테스트 결과 현재 속도가 적절하다면 유지하셔도 됩니다!
⚡ 애니메이션 속도 조정 제안
val state by animateFloatAsState( targetValue = if (checked) maxBound else minBound, - animationSpec = tween(durationMillis = 500), + animationSpec = tween(durationMillis = 200), label = "common switch", )core/ui/src/main/java/com/twix/ui/extension/DismissKeyboardOnTap.kt (1)
3-25: onDismiss 최신 상태 캡처 보장 필요왜 문제인가요:
pointerInput(Unit)은 키가 고정이라onDismiss가 상태에 따라 바뀌어도 이전 람다가 계속 호출될 수 있습니다(예: 화면 상태에 따라 다른 동작을 기대할 때).
어떻게 개선하나요:rememberUpdatedState로 최신 콜백을 참조하거나pointerInput키에onDismiss를 포함해 재시작되도록 해주세요.onDismiss가 변경될 가능성이 있나요?🔧 개선 예시
import androidx.compose.foundation.gestures.detectTapGestures import androidx.compose.runtime.Composable +import androidx.compose.runtime.rememberUpdatedState import androidx.compose.ui.Modifier import androidx.compose.ui.input.pointer.pointerInput import androidx.compose.ui.platform.LocalFocusManager import androidx.compose.ui.platform.LocalSoftwareKeyboardController @@ val focusManager = LocalFocusManager.current val keyboardController = LocalSoftwareKeyboardController.current + val onDismissState = rememberUpdatedState(onDismiss) return this.pointerInput(Unit) { detectTapGestures( onTap = { focusManager.clearFocus() keyboardController?.hide() - onDismiss() + onDismissState.value() }, ) } }core/design-system/src/main/java/com/twix/designsystem/components/dialog/CommonDialog.kt (2)
78-78:fadeDuration상수 중복
fadeDuration = 160이DialogScrim(line 78)과DialogContent(line 106)에서 각각 정의되어 있습니다. 두 값이 동기화되어야 하는 상황에서 하나만 수정하면 애니메이션 타이밍이 어긋날 수 있습니다.♻️ 상수 추출 제안
+private const val DIALOG_FADE_DURATION_MS = 160 + `@Composable` private fun DialogScrim( visible: Boolean, onDismissRequest: () -> Unit, ) { - val fadeDuration = 160 - AnimatedVisibility( visible = visible, - enter = fadeIn(animationSpec = tween(fadeDuration)), - exit = fadeOut(animationSpec = tween(fadeDuration)), + enter = fadeIn(animationSpec = tween(DIALOG_FADE_DURATION_MS)), + exit = fadeOut(animationSpec = tween(DIALOG_FADE_DURATION_MS)),Also applies to: 106-106
135-146:dismissText와onDismiss파라미터 일관성 검토현재
dismissText와onDismiss가 각각 독립적인 nullable 파라미터로 정의되어 있지만, line 135에서 두 값 모두 non-null일 때만 dismiss 버튼이 표시됩니다.이로 인해 호출부에서
dismissText만 전달하거나onDismiss만 전달할 경우 버튼이 표시되지 않아 혼란을 줄 수 있습니다.개선 방안 (선택사항):
- 두 파라미터를 하나의 data class로 묶어 명시적으로 처리
- KDoc 주석으로 두 파라미터를 함께 사용해야 함을 명시
현재 구현이 의도된 것이라면, 문서화를 통해 사용법을 명확히 해주시면 좋겠습니다.
core/design-system/src/main/res/drawable/ic_chevron_down_circle.xml (1)
6-15: 하드코딩된 색상에 대한 Dark Mode 지원 검토가 필요합니다.현재
#171717(배경)과#ffffff(chevron)가 하드코딩되어 있어 Dark Mode에서 테마에 따라 자동으로 변경되지 않습니다.의도된 디자인이라면 괜찮지만, 테마 대응이 필요하다면 다음 방안을 고려해 주세요:
res/drawable-night/에 Dark Mode용 리소스 추가- 또는 테마 색상 attribute 사용 (예:
?attr/colorOnSurface)디자이너와 의도를 확인해 보시는 것을 권장드립니다.
core/design-system/src/main/res/drawable/ic_arrow3_left.xml (1)
6-12: 네비게이션 아이콘의 Dark Mode 지원을 권장드립니다.뒤로 가기 버튼처럼 기능적으로 중요한 네비게이션 아이콘에
#171717이 하드코딩되어 있습니다. Dark Mode에서 어두운 배경 위에 이 색상이 표시되면 거의 보이지 않을 수 있습니다.다음 방안 중 하나를 고려해 주세요:
- 테마 색상 attribute 사용 (권장):
android:strokeColor="?attr/colorOnSurface"
- night 리소스 분리:
res/drawable-night/ic_arrow3_left.xml에 밝은 색상 버전 추가기능적 아이콘이므로 테마 대응을 통해 모든 모드에서 시인성을 확보하는 것이 사용자 경험에 중요합니다.
core/design-system/src/main/res/drawable/ic_minus.xml (1)
10-10: 다크 모드 지원을 위해 하드코딩된 색상 대신 테마 색상 참조를 고려해 주세요.현재
#171717색상이 직접 하드코딩되어 있습니다. 다크 모드에서는 어두운 배경 위에 이 아이콘이 거의 보이지 않게 됩니다.개선 방안:
android:tint속성과 함께 색상 리소스를 사용하거나- Compose에서 아이콘 사용 시
tint파라미터로 테마 색상을 적용하는 방식을 권장합니다.다른 새로 추가된 아이콘들(
ic_plus.xml,ic_health.xml등)에도 동일한 고려가 필요합니다.core/design-system/src/main/res/drawable/ic_health.xml (1)
30-59: 중복된 path 요소들이 있습니다.동일한
pathData를 가진 path 요소들이 반복되고 있습니다:
- Lines 32-34, 36-39, 40-44:
"M2,6a6,3 0,1 0,12 0a6,3 0,1 0,-12 0z"3회 반복- Lines 45-49, 50-54, 55-59:
"M14,6V9C14,10.657..."3회 반복이는 디자인 도구에서 내보낼 때 발생한 것으로 보이며, 불필요한 중복은 앱 크기와 렌더링 성능에 미미하지만 영향을 줄 수 있습니다. 중복된 path를 제거하는 것이 좋습니다.
core/design-system/src/main/res/drawable/ic_exercise.xml (1)
1-31: [전체 아이콘 리소스 요약] 다크 모드 대응 전략을 팀 내에서 논의해 보시면 좋겠습니다.이번 PR에서 추가된 아이콘들은 일러스트 스타일로 다채로운 색상을 사용하고 있습니다. 이런 경우 일반적으로 두 가지 접근 방식이 있습니다:
- 단일 아이콘 유지: 다크/라이트 모드 모두에서 동일한 아이콘 사용 (현재 방식)
- 모드별 아이콘 분리:
drawable-night폴더에 다크 모드용 아이콘 별도 제공브랜드 아이덴티티상 동일한 색상 유지가 필요하다면 현재 방식도 괜찮지만, 가시성 테스트는 꼭 진행해 주세요.
core/design-system/src/main/res/drawable/ic_empty_emoji.xml (1)
12-35: 중복된 path 정의가 있습니다. 의도된 것인지 확인이 필요합니다.동일한
pathData가 여러 번 반복되어 정의되어 있습니다:
- Lines 12-23:
"M10,31L11.5,26H25.5L26,31H10Z"경로가 4번 반복- Lines 24-35:
"M29,31L28.375,26H31L33,31H29Z"경로가 4번 반복마지막
fillColor만 최종적으로 보이므로 불필요한 중복일 수 있습니다. SVG 내보내기 과정에서 생긴 아티팩트인지, 또는 의도된 레이어링인지 확인해 주세요. 불필요하다면 제거하여 리소스 크기를 줄일 수 있습니다.core/design-system/src/main/java/com/twix/designsystem/components/text_field/UnderlineTextField.kt (2)
48-76: 접근성(Accessibility) 개선을 고려해 주세요.
BasicTextField와 별도의 placeholderAppText가 시맨틱하게 연결되어 있지 않습니다. 스크린 리더 사용자를 위해 placeholder 힌트를 제공하는 것이 좋습니다.♿ 접근성 개선 제안
+import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.semantics.contentDescription Box( modifier = Modifier - .padding(horizontal = 8.dp, vertical = 10.dp), + .padding(horizontal = 8.dp, vertical = 10.dp) + .semantics { + if (value.isBlank()) contentDescription = placeHolder + }, contentAlignment = Alignment.CenterStart, ) {As per coding guidelines: 디자인 시스템 리뷰 가이드 - 접근성(Accessibility) 고려 여부
23-82: Preview Composable 추가를 권장합니다.디자인 시스템 컴포넌트에
@Preview함수가 없습니다. Preview를 추가하면 개발 중 컴포넌트의 다양한 상태(빈 값, 입력된 값, disabled 상태 등)를 시각적으로 확인할 수 있습니다.👁️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun UnderlineTextFieldPreview() { UnderlineTextField( value = "", placeHolder = "목표를 입력하세요", onValueChange = {}, ) } `@Preview`(showBackground = true) `@Composable` private fun UnderlineTextFieldFilledPreview() { UnderlineTextField( value = "운동하기", placeHolder = "목표를 입력하세요", onValueChange = {}, ) }As per coding guidelines: Feature 모듈 - MVI 패턴 리뷰 가이드 - Preview Composable이 제공되는가?
feature/goal-editor/src/main/java/com/twix/goal_editor/model/GoalEditorUiState.kt (1)
15-17: 테스트 용이성을 위한 고려사항입니다.
LocalDate.now()를 기본값으로 사용하면 테스트 시 날짜에 의존적인 결과가 발생할 수 있습니다. 현재 구현은 초기 상태 생성에만 영향을 주므로 큰 문제는 아니지만, 테스트 코드 작성 시 명시적으로 날짜를 전달하는 것이 좋습니다.// 테스트 예시 val testState = GoalEditorUiState( startDate = LocalDate.of(2026, 2, 4), endDate = LocalDate.of(2026, 2, 28), )추후 테스트 작성 시 참고해 주세요.
feature/goal-editor/src/main/java/com/twix/goal_editor/component/GoalEditorTopBar.kt (2)
53-61: 접근성:contentDescription에 하드코딩된 영어 문자열 사용
contentDescription이"back"으로 하드코딩되어 있습니다. 스크린 리더 사용자를 위해 다국어 지원이 필요합니다.stringResource를 사용하여 리소스 문자열로 변경하는 것이 좋습니다.♻️ 개선 제안
Image( painter = painterResource(R.drawable.ic_arrow3_left), - contentDescription = "back", + contentDescription = stringResource(R.string.content_description_back), modifier = Modifier .padding(18.dp) .size(24.dp) .noRippleClickable(onClick = onBack), )그리고
strings.xml에 다음을 추가하세요:<string name="content_description_back">뒤로 가기</string>
26-87: Preview Composable 추가 권장디자인 시스템 가이드라인에 따르면 Preview Composable이 제공되어야 합니다. Preview를 추가하면 개발 중 UI 검증이 용이해지고, 다크 모드 등 다양한 상태를 빠르게 확인할 수 있습니다.
♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun GoalEditorTopBarPreview() { GoalEditorTopBar( isEdit = false, onBack = {}, ) } `@Preview`(showBackground = true) `@Composable` private fun GoalEditorTopBarEditModePreview() { GoalEditorTopBar( isEdit = true, onBack = {}, ) }feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt (1)
61-70:save()함수의 실제 저장 로직 구현 필요현재
save()함수는 유효성 검사만 수행하고 실제 저장 로직이 구현되어 있지 않습니다. PR 설명에 따르면 추후 구현 예정인 것으로 보이지만, TODO 주석을 추가하여 미완성 상태임을 명시하면 다른 개발자가 코드를 이해하는 데 도움이 됩니다.TODO 주석 추가를 원하시면 말씀해 주세요. 또는 저장 로직 구현을 위한 이슈를 생성해 드릴 수 있습니다.
feature/goal-editor/src/main/java/com/twix/goal_editor/component/GoalInfoCard.kt (5)
247-253:RepeatType.label()확장 함수 위치 검토
RepeatType.label()확장 함수가@Composable로 선언되어 있고 public으로 노출되어 있습니다. 이 함수는 다른 곳에서도 재사용될 수 있으므로, 별도의 파일(예:RepeatTypeExtensions.kt)로 분리하거나 domain 모듈의RepeatType근처로 이동하는 것을 고려해 보세요.현재 위치에서는
GoalInfoCard.kt를 찾아봐야만 이 확장 함수를 발견할 수 있어 코드 탐색이 어려울 수 있습니다.
155-159: 하드코딩된 포맷 문자열을 리소스로 이동 권장
"%s %s번"형태의 포맷 문자열이 하드코딩되어 있습니다. 다국어 지원과 일관성을 위해strings.xml의 포맷 문자열을 사용하는 것이 좋습니다.♻️ 개선 제안
strings.xml에 추가:<string name="repeat_count_format">%1$s %2$d번</string>코드 변경:
AppText( - text = "%s %s번".format(selectedRepeatType.label(), repeatCount), + text = stringResource(R.string.repeat_count_format, selectedRepeatType.label(), repeatCount), style = AppTextStyle.B2, color = GrayColor.C500, )
198-202: 날짜 포맷의 다국어 지원 검토
"%s월 %s일"형태의 날짜 포맷이 하드코딩되어 있습니다. 한국어에서는 자연스럽지만, 향후 다국어 지원 시 각 언어별로 날짜 표현 방식이 다를 수 있습니다.
DateTimeFormatter와strings.xml의 포맷 문자열을 조합하거나, Android의DateUtils또는java.time.format.DateTimeFormatter.ofLocalizedDate()를 사용하는 것을 고려해 보세요.♻️ 대안 예시
// strings.xml <string name="date_format_month_day">%1$d월 %2$d일</string> // 또는 DateTimeFormatter 사용 val formatter = DateTimeFormatter.ofPattern("M월 d일", Locale.KOREA) text = date.format(formatter)
161-167:contentDescription접근성 개선 필요이미지의
contentDescription이"repeat type","date"등 영어로 하드코딩되어 있습니다. 스크린 리더 사용자를 위해 의미 있는 한국어 설명을stringResource로 제공하는 것이 좋습니다.♻️ 개선 제안
Image( painter = painterResource(R.drawable.ic_chevron_down_circle), - contentDescription = "repeat type", + contentDescription = stringResource(R.string.content_description_select_repeat_count), ... )Image( painter = painterResource(R.drawable.ic_chevron_down_circle), - contentDescription = "date", + contentDescription = stringResource(R.string.content_description_select_date), ... )Also applies to: 204-210
40-96: Preview Composable 추가 권장
GoalInfoCard는 복잡한 UI 컴포넌트입니다. Preview를 추가하면 다양한 상태(endDateEnabled true/false, 다른 RepeatType 등)에서의 렌더링을 빠르게 확인할 수 있습니다.♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun GoalInfoCardPreview() { GoalInfoCard( selectedRepeatType = RepeatType.DAILY, repeatCount = 1, startDate = LocalDate.now(), endDateEnabled = false, endDate = LocalDate.now().plusMonths(1), onSelectedRepeatType = {}, onShowRepeatCountBottomSheet = {}, onShowCalendarBottomSheet = {}, onToggleEndDateEnabled = {}, ) } `@Preview`(showBackground = true) `@Composable` private fun GoalInfoCardWithEndDatePreview() { GoalInfoCard( selectedRepeatType = RepeatType.WEEKLY, repeatCount = 3, startDate = LocalDate.now(), endDateEnabled = true, endDate = LocalDate.now().plusMonths(1), onSelectedRepeatType = {}, onShowRepeatCountBottomSheet = {}, onShowCalendarBottomSheet = {}, onToggleEndDateEnabled = {}, ) }core/design-system/src/main/res/drawable/ic_pencil.xml (1)
1-75: 다크 모드/테마 대응 여부를 한 번 확인해 주세요.
현재 모든 색상이 HEX 하드코딩이라 다크 모드에서 대비가 떨어질 수 있습니다. 필요하면@color/...리소스로 분리하고 night 변형을 두는 방향이 안전합니다.As per coding guidelines, “Dark Mode 지원 여부” 및 “색상과 테마가 디자인 시스템에 포함되는가?”.
core/design-system/src/main/res/drawable/ic_laptop.xml (1)
6-32: 동일pathData중복으로 불필요한 overdraw가 생깁니다.
같은pathData가 여러 번 반복되어 최종 출력은 마지막 색만 남고 앞선 레이어는 의미가 없습니다. 의도된 레이어링이 아니라면 중복 제거를 권장합니다. 혹시 색상 레이어링 의도인가요?♻️ 중복 path 제거 예시
- <path - android:pathData="M5.039,25.769L6.468,21.004H19.809L20.286,25.769H5.039Z" - android:fillColor="#FF96AD"/> - <path - android:pathData="M5.039,25.769L6.468,21.004H19.809L20.286,25.769H5.039Z" - android:fillColor="#FF96AD"/> + <path + android:pathData="M5.039,25.769L6.468,21.004H19.809L20.286,25.769H5.039Z" + android:fillColor="#FF96AD"/>feature/goal-editor/src/main/java/com/twix/goal_editor/component/EmojiPicker.kt (1)
44-55:toRes()함수에서@Composable어노테이션이 불필요합니다.이 함수는 Compose 런타임 기능(
remember,LaunchedEffect등)을 사용하지 않고 단순 매핑만 수행하므로, 일반 확장 함수로 선언하는 것이 적절합니다.@Composable로 선언하면 Composable 컨텍스트 외부에서 호출할 수 없는 불필요한 제약이 생깁니다.♻️ 일반 확장 함수로 변경
-@Composable fun GoalIconType.toRes(): Int = when (this) { GoalIconType.DEFAULT -> R.drawable.ic_defaultfeature/goal-editor/src/main/java/com/twix/goal_editor/component/GoalTextField.kt (1)
22-48: 포커스 기반 커밋 패턴이 잘 구현되어 있습니다.
wasFocused플래그를 사용하여 초기 무의미한 커밋을 방지하고, 포커스 해제 시에만 값을 커밋하는 패턴이 UX 측면에서 적절합니다.rememberSaveable(value)를 키로 사용하여 외부 상태 변경 시 내부 상태가 동기화되는 것도 올바른 접근입니다.다만, UI 컴포넌트의 독립적인 테스트와 디자인 확인을 위해
@PreviewComposable을 추가하면 좋겠습니다.💡 Preview 추가 제안 (선택사항)
`@Preview`(showBackground = true) `@Composable` private fun GoalTextFieldPreview() { TwixTheme { GoalTextField( value = "목표 제목", onCommitTitle = {}, ) } }feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorScreen.kt (1)
267-273: 접근성(Accessibility)을 위한contentDescription개선을 제안합니다.현재
contentDescription이 "emoji"로 하드코딩되어 있어, 스크린 리더 사용자가 어떤 아이콘인지 구분하기 어렵습니다. 각 아이콘 타입에 맞는 설명을 제공하면 접근성이 향상됩니다.♻️ 접근성 개선 제안
GoalIconType에 label을 추가하거나, 기존 리소스를 활용하여 의미 있는 설명을 제공하세요:Image( painter = painterResource(it.toRes()), - contentDescription = "emoji", + contentDescription = it.name.lowercase(), modifier =또는 별도의 확장 함수로 각 아이콘에 대한 설명을 정의할 수 있습니다.
core/design-system/src/main/java/com/twix/designsystem/components/common/CommonSwitch.kt
Show resolved
Hide resolved
core/design-system/src/main/java/com/twix/designsystem/components/dialog/CommonDialog.kt
Show resolved
Hide resolved
core/design-system/src/main/java/com/twix/designsystem/components/dialog/CommonDialog.kt
Show resolved
Hide resolved
...esign-system/src/main/java/com/twix/designsystem/components/text_field/UnderlineTextField.kt
Show resolved
Hide resolved
feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorScreen.kt
Show resolved
Hide resolved
feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorScreen.kt
Show resolved
Hide resolved
feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt
Show resolved
Hide resolved
chanho0908
left a comment
chanho0908
left a comment
There was a problem hiding this comment.
고생했어 현수야 !
이미지 파일 때문에 크기가 어마 무시해졌군 ㅇㅅㅇ ,,,
전체적으로 테스트 해봤을 때 문제 없어서 작은 의견 하나만 남겼어 !
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| AppText( | ||
| text = "%s월 %s일".format(date.monthValue, date.dayOfMonth), |
There was a problem hiding this comment.
strings resource로 분리할 수 있을 것 같아 !
| viewModelScope.launch { | ||
| emitSideEffect(GoalEditorSideEffect.ShowToast("종료 날짜가 시작 날짜보다 이전입니다.", ToastType.ERROR)) | ||
| } |
There was a problem hiding this comment.
ViewModel은 SideEffect만 발생시키고 이를 어떻게 처리할지는 Ui에서 관리하도록 관심사를 분리하는건 어떨까 ?
출력할 메시지를 Ui에서 관리하면 strings로도 분리할 수 있어서 추후 다국어 대응할 때도 좀 더
유지보수성이 편리해질 것 같아 !
| @Composable | ||
| fun UnderlineTextField( |
There was a problem hiding this comment.
온보딩 닉네임 입력/날짜 선택 화면에서 trailing Icon이 필요한 경우가 있어서 추가해줄 수 있을까 ?
2 participants
이슈 번호
작업내용
결과물
2026-02-04.5.40.17.mov
리뷰어에게 추가로 요구하는 사항 (선택)
수정은 이후에 홈 화면에서 수정으로 넘어가는 기능 구현할 때 네비게이션 파라미터 넘기는 방식으로 뷰모델에서 상태를 초기화시키는 로직을 구현할 예정입니다!
디자인 상으로는 바텀시트에서 목표 생성으로 넘어가야 하는데 이것도 홈 화면 작업 이후에 바텀시트를 중간에 끼워넣는 걸로 작업할 예정입니다.