[FEAT/#26] Checkbox 컴포넌트 구현#27
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCompose 기반의 ChangesCheckbox 컴포넌트
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SsingCheckbox
participant State as checked State
User->>SsingCheckbox: 클릭 (toggleable)
SsingCheckbox->>State: onCheckedChange 호출
State-->>SsingCheckbox: checked 값 갱신
SsingCheckbox->>SsingCheckbox: 배경/테두리 색상 애니메이션
SsingCheckbox->>SsingCheckbox: 체크 아이콘 AnimatedVisibility 전환
관련 이슈: 제안 레이블: enhancement, ui 제안 리뷰어: 없음 (정보 부족) 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/ui/src/main/java/com/ssing/core/ui/common/component/SsingCheckbox.kt (1)
89-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreview에 라이트/다크/폰트 스케일 변형 추가 필요
현재
@Preview가 기본 설정 하나만 제공됩니다. 체크/언체크 상태 조합과 함께 라이트/다크 테마 변형도 함께 제공하면 디자인 검증에 도움이 됩니다.As per path instructions, "
@Preview라이트/다크/폰트 변형 제공" 가이드를 따르지 않았습니다.♻️ 제안
-@Preview +@Preview(name = "Light", uiMode = Configuration.UI_MODE_NIGHT_NO) +@Preview(name = "Dark", uiMode = Configuration.UI_MODE_NIGHT_YES) `@Composable` private fun SsingCheckboxPreview() { SSINGTheme { var checked by remember { mutableStateOf(false) } SsingCheckbox( checked = checked, onCheckedChange = { checked = it } ) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/ui/src/main/java/com/ssing/core/ui/common/component/SsingCheckbox.kt` around lines 89 - 99, The current SsingCheckboxPreview only renders one default `@Preview`, so update SsingCheckboxPreview to cover both checked and unchecked states and add preview variants for light/dark theme and font scale changes. Use the existing SsingCheckbox composable and SSINGTheme in the preview setup, and add separate preview annotations or helper preview composables so design can validate the same component across those combinations.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/ui/src/main/java/com/ssing/core/ui/common/component/SsingCheckbox.kt`:
- Around line 89-99: The current SsingCheckboxPreview only renders one default
`@Preview`, so update SsingCheckboxPreview to cover both checked and unchecked
states and add preview variants for light/dark theme and font scale changes. Use
the existing SsingCheckbox composable and SSINGTheme in the preview setup, and
add separate preview annotations or helper preview composables so design can
validate the same component across those combinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c67036fe-a6d7-422f-aeaf-660556881b1c
⛔ Files ignored due to path filters (1)
core/ui/src/main/res/drawable/ic_check.xmlis excluded by none and included by none
📒 Files selected for processing (1)
core/ui/src/main/java/com/ssing/core/ui/common/component/SsingCheckbox.kt
doorimng
left a comment
doorimng
left a comment
There was a problem hiding this comment.
와웅 애니메이션까지!! 짱짱 공컴의 달인 예슬언니
애니메이션 넣은 거 좋은 것 같아용!
| */ | ||
| @Composable | ||
| fun SsingCheckbox( | ||
| checked: Boolean, |
There was a problem hiding this comment.
p2:
요기는 Boolean 타입의 변수이고 버튼이 체크되었는지의 여부를 나타내니까 isChecked 같은 이름은 어떨깡용??
There was a problem hiding this comment.
오 그렇네요 좋아요!! 수정하겠습니다 ㅎㅎ 감사해요!
| val backgroundColor by animateColorAsState( | ||
| targetValue = if (checked) SSINGTheme.colors.primaryNormal else SSINGTheme.colors.backgroundNormal, | ||
| animationSpec = tween(100), | ||
| ) | ||
|
|
||
| val borderColor by animateColorAsState( | ||
| targetValue = if (checked) SSINGTheme.colors.primaryAlternative else SSINGTheme.colors.borderNormal, | ||
| animationSpec = tween(100), | ||
| ) |
There was a problem hiding this comment.
p2: 히히 이거 확장프로퍼티로 구현해보는 건 어때용 스타일 맞추면 좋을 것 같우다!!
There was a problem hiding this comment.
구분이 둘 밖에 없어서 고민했는데 말씀대로 스타일 맞추는 게 더 좋겠군요 !! 수정할게요 감사합니다!
| AnimatedVisibility( | ||
| visible = checked, | ||
| enter = fadeIn(tween(100)), | ||
| exit = fadeOut(tween(100)), | ||
| ) { |
There was a problem hiding this comment.
p2: 프리뷰에 체크 했을 때랑 안 했을 때랑 둘 다 보여주면 좋겠다... 언니 심심해서 할 거 없을 때 해주면 좋겠다...
There was a problem hiding this comment.
인터렉션 모드로 전환하면 직접 클릭해서 체크 가능해요! 디자인 두 개 화면에 직접 띄우는 쪽이 더 보기에 편할까요?
There was a problem hiding this comment.
지금 예슬 언니 같은 경우는 클릭으로 아이콘이 자연스럽게 전환되는지 인터랙션에 신경써서 애니메이션까지 넣어둔 컴포인지라 클릭 기능 있는 프리뷰 하나만 둔 것도 완전 이해됨!!
저는 개발을 하다보니 내가 만든 컴포 잘 돌아가나?를 확인하는 목적도 있지만,
맞다 이 컴포 이 모양이었지~하고 한 눈에 파악할 수 있을 때 프리뷰가 너무 유용하더라구요!
그래서 전 개인적으로 컴포가 가질 수 있는 모든 상태 별로 프리뷰를 만들긴 해용 ㅎㅎ
private class SsingCheckboxPreviewProvider: PreviewParameterProvider<Boolean> {
override val values: Sequence<Boolean>
get() = sequenceOf(true, false)
}
@Preview
@Composable
private fun SsingCheckboxPreview(
@PreviewParameter(SsingCheckboxPreviewProvider::class) intialChecked: Boolean,
) {
SSINGTheme {
var checked by remember { mutableStateOf(intialChecked) }이상 프리뷰 파라미터 신봉자
| AnimatedVisibility( | ||
| visible = checked, | ||
| enter = fadeIn(tween(100)), | ||
| exit = fadeOut(tween(100)), | ||
| ) { |
| ) { | ||
| Icon( | ||
| imageVector = ImageVector.vectorResource(R.drawable.ic_check), | ||
| contentDescription = null, |
There was a problem hiding this comment.
p3: 개인적으로 궁금한건데, contentDescription = null을 설정하는 기준이 혹시 따로 있을까요?? 아니면 항상 null으로 하시나용
There was a problem hiding this comment.
이거 그 스크린리더에서 인식해주는 거라서 아이콘이 단순 디자인 요소이거나 옆에 라벨이 있는 경우 등 보통의 경우에 null로 두는 것으로 알고 있어요! 대부분 시각적 장식 역할이니까요
이전에 웬만하면 여기 null로 비워두라는 피드백을 들었던 기억이,, 있습니당
doyeon0307
left a comment
doyeon0307
left a comment
There was a problem hiding this comment.
짱짱!! 이런 조구만 체크박스도 그냥 넘어가지 않고 애니메이션 넣어주는 섬세함에 감동띠니
코리 단 건 엄청 중요한 건 아니고 알아두면 좋다 정도라 바로 어푸해요!!
바로 반영 안 해도 완전 괜찮은데 기억은 해두기 ㅎㅎ
There was a problem hiding this comment.
지금 예슬 언니 같은 경우는 클릭으로 아이콘이 자연스럽게 전환되는지 인터랙션에 신경써서 애니메이션까지 넣어둔 컴포인지라 클릭 기능 있는 프리뷰 하나만 둔 것도 완전 이해됨!!
저는 개발을 하다보니 내가 만든 컴포 잘 돌아가나?를 확인하는 목적도 있지만,
맞다 이 컴포 이 모양이었지~하고 한 눈에 파악할 수 있을 때 프리뷰가 너무 유용하더라구요!
그래서 전 개인적으로 컴포가 가질 수 있는 모든 상태 별로 프리뷰를 만들긴 해용 ㅎㅎ
private class SsingCheckboxPreviewProvider: PreviewParameterProvider<Boolean> {
override val values: Sequence<Boolean>
get() = sequenceOf(true, false)
}
@Preview
@Composable
private fun SsingCheckboxPreview(
@PreviewParameter(SsingCheckboxPreviewProvider::class) intialChecked: Boolean,
) {
SSINGTheme {
var checked by remember { mutableStateOf(intialChecked) }이상 프리뷰 파라미터 신봉자
| val backgroundColor by animateColorAsState( | ||
| targetValue = isChecked.backgroundColor, | ||
| animationSpec = tween(100), | ||
| ) | ||
|
|
||
| val borderColor by animateColorAsState( | ||
| targetValue = isChecked.borderColor, | ||
| animationSpec = tween(100), | ||
| ) |
There was a problem hiding this comment.
p2 : 트리거가 동일한 두 애니메이션이 독립적으로 구현되어있어요.
동시에 발동해야 하는 애니메이션을 구현할 때는Transition 기억하기!! 상태 동기화가 보장되고 관리/디버깅이 편리해집니다.
| val backgroundColor by animateColorAsState( | |
| targetValue = isChecked.backgroundColor, | |
| animationSpec = tween(100), | |
| ) | |
| val borderColor by animateColorAsState( | |
| targetValue = isChecked.borderColor, | |
| animationSpec = tween(100), | |
| ) | |
| val transition = updateTransition(targetState = isChecked, label = "checkedTransition") | |
| val backgroundColor by transition.animateColor( | |
| transitionSpec = { tween(100) }, | |
| label = "backgroundColor" | |
| ) { checked -> checked.backgroundColor } | |
| val borderColor by transition.animateColor( | |
| transitionSpec = { tween(100) }, | |
| label = "borderColor" | |
| ) { checked -> checked.borderColor } |
| ) { | ||
| val backgroundColor by animateColorAsState( | ||
| targetValue = isChecked.backgroundColor, | ||
| animationSpec = tween(100), |
There was a problem hiding this comment.
p2 : 현재 모든 애니메이션 durataion이 동일 값으로 하드코딩 되어 있어 duration을 바꾸고 싶을 때 코드 4군데를 직접 수정해야해 유지보수가 어렵습니다. 상수 분리 추천드려용
| animationSpec = tween(100), | |
| private const val ANIMATION_DURATION = 100 | |
| animationSpec = tween(ANIMATION_DURATION), |
5 participants
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
ic_check아이콘 색상이 atomic 토큰으로 설정되어있어White토큰 직접 사용하였습니다.Summary by CodeRabbit