[NDGL-51] 디자인 시스템 컴포넌트 구현(네비게이션바, ChipTab, Button, Modal)#12
[NDGL-51] 디자인 시스템 컴포넌트 구현(네비게이션바, ChipTab, Button, Modal)#12
Conversation
Walkthrough베이스 패키지를 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
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
🤖 Fix all issues with AI agents
In
`@core/ui/src/main/java/com/yapp/ndgl/core/ui/designSystem/NDGLNavigationBar.kt`:
- Around line 54-63: The interactive Icons in NDGLNavigationBar currently set
contentDescription = null which breaks accessibility; update the
NDGLNavigationBar composable signature to accept optional String parameters
(e.g., leadingIconContentDescription and trailingIconContentDescription) and
pass those values into the Icon composables instead of null (the Icon in the
leadingIcon block and the trailing Icon usage tied to onLeadingIconClick /
onTrailingIconClick). Ensure the new params are documented or have sensible
defaults and are forwarded to ImageVector.vectorResource Icon contentDescription
so screen readers get meaningful labels.
- Around line 43-44: NDGLNavigationBar의 파라미터인 trailingIcons는 다수 아이콘을 받는데
onTrailingIconClick이 단일 콜백이라 어떤 아이콘이 눌렸는지 구분할 수 없습니다; onTrailingIconClick의 시그니처를
onTrailingIconClick: (index: Int) -> Unit 으로 변경하고 기본값을 { }로 유지한 뒤,
NDGLNavigationBar 내부에서 각 아이콘 클릭 리스너를 설정할 때 아이콘의 인덱스(예: forEachIndexed의 index)를
파라미터로 전달하도록 수정하세요; 또한 이 변경에 따라 NDGLNavigationBar를 호출하는 모든 사용처의 호출부를 인덱스 기반 콜백을
받도록 업데이트하세요.
🧹 Nitpick comments (5)
core/ui/src/main/java/com/yapp/ndgl/core/ui/designSystem/NDGLCTAButton.kt (2)
86-89: 접근성: 버튼에 semantic role 추가 필요
clickablemodifier에role = Role.Button을 추가하여 스크린 리더가 이 요소를 버튼으로 인식할 수 있도록 해야 합니다. 이는 접근성 요구사항을 충족하는 데 중요합니다.♻️ 제안된 수정
+import androidx.compose.ui.semantics.Role.clickable( enabled = status != NDGLCTAButtonAttr.Status.DISABLED, onClick = onClick, + role = Role.Button, )
90-90: 하드코딩된 vertical padding 검토 필요
vertical = 8.dp가 하드코딩되어 있는 반면, horizontal padding은 Size enum에서 관리됩니다. SMALL 사이즈(높이 32.dp)에서 vertical padding 8.dp를 적용하면 콘텐츠 영역이 16.dp만 남게 됩니다. 일관성을 위해 vertical padding도 Size enum에 추가하는 것을 고려해 보세요.core/ui/src/main/java/com/yapp/ndgl/core/ui/designSystem/NDGLNavigationBar.kt (1)
79-89: 미사용 변수:index가 사용되지 않음
forEachIndexed를 사용하지만index를 사용하지 않습니다. 단, 앞서 제안한 인덱스 기반 콜백으로 변경하면 이index가 필요하게 됩니다. 그렇지 않다면forEach로 변경하세요.현재 API 유지 시 forEach 사용
- trailingIcons?.forEachIndexed { index, icon -> + trailingIcons?.forEach { icon ->core/ui/src/main/java/com/yapp/ndgl/core/ui/designSystem/NDGLOutlinedButton.kt (1)
45-59: OutlinedButton 사용 검토로 버튼 접근성/상호작용 일관성 확보Row + clickable로 직접 구현하면 버튼 역할/포커스/상호작용(리플, 최소 터치 타깃 등)이 기본 보장되지 않을 수 있습니다. Material3
OutlinedButton으로 대체하거나, 동일 수준의 접근성/상호작용을 명시적으로 보강하는 방향을 권장합니다.core/ui/src/main/java/com/yapp/ndgl/core/ui/designSystem/NDGLChipTab.kt (1)
73-79: 탭 선택 상태를 접근성 의미론으로 올바르게 노출해주세요.
clickable대신selectable+Role.Tab을 사용하면 스크린리더/키보드 네비게이션에서 선택 상태가 올바르게 전달됩니다. 또한 외부 Row 컨테이너에.selectableGroup()을 적용하여 탭들이 상호 배타적인 그룹임을 명시해야 합니다.♿ 제안 변경안
NDGLChipTabItem에서:+import androidx.compose.foundation.selection.selectable +import androidx.compose.ui.semantics.Role- .clickable(onClick = onTabSelected) + .selectable( + selected = isSelected, + onClick = onTabSelected, + role = Role.Tab, + )
NDGLChipTab의 외부 Row에서:+import androidx.compose.foundation.selection.selectableGroupRow( modifier = modifier.horizontalScroll(scrollState), + .selectableGroup() horizontalArrangement = Arrangement.spacedBy(8.dp), ) {
|
수고하셨습니다! |
|
또 그때 말한 것처럼 현재 pretendard semibold가 잘 적용이 되지 않는 것 같습니다 .. |
이거 두 이슈랑 디자인 시스템 색상 변경된 것들 커밋 붙여서 올리겠습니다! 올리고 다시 코멘트 달게요!! |
|
현재 primary 50 ~ 900 색상이 Fimga DesignSystem에 명시된 것과 다른 것 같습니다. |
https://discord.com/channels/1447624057477398794/1464500560974839879/1464500756102123541 |
jihee-dev
force-pushed
the
feature/NDGL-51
branch
4 times, most recently
from
cb4248e to
be4674e
Compare
jihee-dev
force-pushed
the
feature/NDGL-51
branch
2 times, most recently
from
50809af to
9e42a96
Compare
|
font weight가 제대로 적용되지 않는 이슈 두 이슈 수정 후 올려두었습니다! @mj010504 |
|
https://developer.android.com/kotlin/style-guide?hl=ko |
오 제가 봤었던 건 kotlin 공식 사이트에 작성된 컨벤션이었는데
|
넵 저는 3단어 이상 사용 자체를 금지하고 2단어 정도면 붙이는 것도 괜찮지 않을까 싶습니다. 저도 뭘 따라야할지 혼란스럽네요.. |
jihee-dev
changed the title
jihee-dev
force-pushed
the
feature/NDGL-51
branch
from
c8c45fd to
8c2ec7f
Compare
좋습니다! 사실 어느 쪽을 따르든 큰 차이는 없을 것 같아서 소문자로 통일하는 걸로 하시죠~! 수정해 뒀습니다! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/ui/src/main/java/com/yapp/ndgl/core/ui/designSystem/NDGLNavigationBar.kt`:
- Line 1: 파일 NDGLNavigationBar.kt의 패키지 선언(package
com.yapp.ndgl.core.ui.designsystem)과 실제 디렉토리명(designSystem) 불일치로 Detekt/CI가
실패합니다; NDGLNavigationBar.kt 내부의 패키지 선언을 실제 폴더명과 일치시키거나(예: package
com.yapp.ndgl.core.ui.designSystem) 해당 폴더명을 소문자 패키지 규칙에 맞게 변경(예:
designsystem)하세요, 변경 후 동일 폴더에 있는 다른 새 파일들의 패키지 선언도 동일하게 일괄 수정하고 빌드/검증(Detekt) 통과
여부를 확인하세요.
8c2ec7f to
ef6590f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/ui/src/main/java/com/yapp/ndgl/core/ui/designsystem/NDGLModal.kt`:
- Around line 80-98: The Column uses verticalArrangement =
Arrangement.spacedBy(16.dp) while the Text composables for body (Text with
variable body) and description (inside description?.let) also add padding(top =
16.dp / 12.dp), causing doubled spacing; choose one approach and fix it by
removing the top paddings on the body and description Text nodes (or
alternatively remove/adjust the Arrangement.spacedBy in the Column) so spacing
is applied in a single place—target the Column's verticalArrangement
(Arrangement.spacedBy) and the two Text instances named for body and description
to make them consistent.
🧹 Nitpick comments (1)
core/ui/src/main/java/com/yapp/ndgl/core/ui/designsystem/NDGLChipTab.kt (1)
134-149: Preview에서 모든 탭의tag값이 동일합니다.테스트/프리뷰 데이터지만,
tag값이 모두"1"로 동일하여 실제 사용 시나리오와 다릅니다. 각 탭에 고유한 tag 값을 부여하는 것이 좋습니다.♻️ 프리뷰 데이터 수정 제안
NDGLChipTab( tabs = persistentListOf( NDGLChipTabAttr.Tab( - tag = "1", + tag = "day1", name = "1일차", leadingIcon = R.drawable.ic_20_tv, ), NDGLChipTabAttr.Tab( - tag = "1", + tag = "day2", name = "2일차", leadingIcon = R.drawable.ic_20_tv, ), NDGLChipTabAttr.Tab( - tag = "1", + tag = "day3", name = "3일차", ), ),
jihee-dev
force-pushed
the
feature/NDGL-51
branch
from
ef6590f to
6f315bd
Compare
jihee-dev
force-pushed
the
feature/NDGL-51
branch
from
6f315bd to
af57922
Compare
|
@mj010504 님 코드래빗 리뷰 하나 수정했고, 제가 커밋 메시지를 룰에 맞지 않게 푸시 한 것들이 있어서 수정했는데 요고만 확인 부탁드립니다 ㅠㅠ 어프로브 주셨는데 이걸 지금 발견해가지고.. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/ui/src/main/java/com/yapp/ndgl/core/ui/theme/Typography.kt`:
- Around line 21-26: Remove the now-unnecessary
`@OptIn`(ExperimentalTextApi::class) annotation above the
FontWeight.pretendardFont() function; simply delete the annotation line and keep
the function as-is (FontWeight.pretendardFont, FontVariation.Settings,
FontVariation.weight) so the code uses the stable FontVariation API without
experimental opt-in.
🧹 Nitpick comments (5)
core/ui/src/main/java/com/yapp/ndgl/core/ui/designsystem/NDGLOutlinedButton.kt (1)
3-22: 아이콘 리소스 타입 안전성 + 버튼 접근성 Role 추가를 권장합니다.
ImageVector.vectorResource(@DrawableRes)는 벡터가 아닌 drawable 전달 시 런타임 크래시가 날 수 있어painterResource로 완화하거나, 파라미터 타입을ImageVector로 제한하는 편이 안전합니다. 또한clickable에role = Role.Button을 지정하면 접근성 안내가 개선됩니다.🔧 제안 변경
-import androidx.compose.ui.graphics.vector.ImageVector -import androidx.compose.ui.res.vectorResource +import androidx.compose.ui.res.painterResource +import androidx.compose.ui.semantics.Role ... - Icon( - imageVector = ImageVector.vectorResource(icon), + Icon( + painter = painterResource(id = icon), contentDescription = null, modifier = Modifier.size(20.dp), tint = contentColor, ) ... - .clickable( + .clickable( enabled = status != NDGLOutlinedButtonAttr.Status.DISABLED, + role = Role.Button, onClick = onClick, ) ... - Icon( - imageVector = ImageVector.vectorResource(icon), + Icon( + painter = painterResource(id = icon), contentDescription = null, modifier = Modifier.size(20.dp), tint = contentColor, )Also applies to: 45-88
core/ui/src/main/java/com/yapp/ndgl/core/ui/designsystem/NDGLCTAButton.kt (2)
29-66: SMALL 높이(32dp) 터치 타깃 검증 필요합니다.
Material 접근성 기준(최소 48dp)보다 작아 실제 터치 영역 부족 위험이 있습니다. 시각 크기는 유지하되minimumInteractiveComponentSize()또는sizeIn(minHeight = 48.dp)같은 보완을 고려해 주세요.
3-25: 아이콘 리소스 타입 안전성 + 버튼 Role 지정 권장.
ImageVector.vectorResource(@DrawableRes)는 벡터가 아닌 drawable 전달 시 크래시 가능성이 있어painterResource로 변경하거나 파라미터 타입을ImageVector로 제한하는 편이 안전합니다. 또한clickable에role = Role.Button을 추가하면 접근성이 향상됩니다.🔧 제안 변경
-import androidx.compose.ui.graphics.vector.ImageVector -import androidx.compose.ui.res.vectorResource +import androidx.compose.ui.res.painterResource +import androidx.compose.ui.semantics.Role ... - Icon( - imageVector = ImageVector.vectorResource(icon), + Icon( + painter = painterResource(id = icon), contentDescription = null, modifier = Modifier.size(size.iconSize), tint = contentColor, ) ... - .clickable( + .clickable( enabled = status != NDGLCTAButtonAttr.Status.DISABLED, + role = Role.Button, onClick = onClick, ) ... - Icon( - imageVector = ImageVector.vectorResource(icon), + Icon( + painter = painterResource(id = icon), contentDescription = null, modifier = Modifier.size(size.iconSize), tint = contentColor, )Also applies to: 81-120
core/ui/src/main/java/com/yapp/ndgl/core/ui/theme/Typography.kt (1)
14-26: 가변 폰트 지원을 위한 좋은 접근 방식입니다.
FontVariation.Settings를 사용하여 Pretendard 폰트의 weight 문제를 해결한 것은 적절합니다. PR에서 언급된 semibold 폰트 미적용 이슈가 이 변경으로 해결될 것입니다.한 가지 가독성 개선 제안: Line 25의
weight가FontWeight의weightInt 속성을 참조하는 것인지 직관적이지 않을 수 있습니다. 명시적으로 작성하면 더 명확해집니다.♻️ 선택적 개선 제안
`@OptIn`(ExperimentalTextApi::class) private fun FontWeight.pretendardFont() = Font( R.font.pretendard, weight = this, - variationSettings = FontVariation.Settings(FontVariation.weight(weight)), + variationSettings = FontVariation.Settings(FontVariation.weight(this.weight)), )core/ui/src/main/java/com/yapp/ndgl/core/ui/designsystem/NDGLNavigationBar.kt (1)
59-69: 헤드라인은 단일 라인 + 말줄임 처리 권장.
긴 타이틀이 48dp 높이에서 줄바꿈/클리핑될 수 있어maxLines/overflow지정이 안전합니다. 긴 문구로 Preview 확인 부탁드립니다.♻️ 제안 변경
-import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.text.style.TextOverflow ... Text( text = text, modifier = Modifier.weight(1f), style = NDGLTheme.typography.bodyLgMedium, color = NDGLTheme.colors.black700, textAlign = when (textAlignType) { NDGLNavigationBarAttr.TextAlignType.START -> TextAlign.Start NDGLNavigationBarAttr.TextAlignType.CENTER -> TextAlign.Center }, + maxLines = 1, + overflow = TextOverflow.Ellipsis, )
넵 수고하셨습니다. 머지해도될 것 같습니다! |
2 participants
NDGL-51 디자인 시스템 컴포넌트 구현(네비게이션바, ChipTab, Button)
연관 문서
디자인
변경사항
Summary by CodeRabbit
릴리스 노트
New Features
Style
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.