[FEAT/#3] 디자인 시스템 구현#4
Conversation
Walkthrough컬러와 타이포그래피를 불변 데이터 클래스 및 CompositionLocal로 재구성하고, 공개 CherrishTheme API와 ProvideCherrishColorsAndTypography 컴포저블로 색상·타이포그래피를 공급하며 SideEffect로 상태표시줄 외관을 설정합니다. (50단어 이내) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Composable
participant Provider as ProvideCherrishColorsAndTypography
participant Locals as CompositionLocals
participant Material as MaterialTheme
participant Activity as Activity/Window
App->>Provider: 호출(content)
Provider->>Locals: CompositionLocalProvider(colors, typography)
Locals->>Material: MaterialTheme(...)으로 래핑
Material->>App: UI 렌더링 (CherrishTheme.colors/typography 사용)
Provider->>Activity: SideEffect -> 상태표시줄 외관 설정
Note right of Provider: 색상·타이포그래피를 CompositionLocal로 제공
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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 @app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt:
- Around line 66-83: The extension function toMaterialColorScheme currently
ignores its receiver and directly references CherrishTheme.colors; update the
body of toMaterialColorScheme to use the receiver (e.g., replace
CherrishTheme.colors.CherrishPink, .Gray0, .Red700, etc. with this.CherrishPink
/ this.Gray0 / this.Red700 or simply CherrishPink when unambiguous) so the
passed CherrishColors instance is used, or alternatively convert the extension
into a regular function that accepts a CherrishColors parameter if you intend to
always use the global theme.
- Around line 48-56: The cast "(view.context as Activity)" is unsafe; replace it
with a safe lookup that unwraps ContextWrappers to find an Activity before using
it (e.g., from LocalView.current get view, guard !view.isInEditMode, then in
SideEffect attempt to resolve Activity by walking context wrappers and only call
activity.window.run { WindowCompat.getInsetsController(this,
view).isAppearanceLightStatusBars = true } if an Activity was found); ensure you
handle the null case by skipping the window/insets call rather than casting
directly.
🧹 Nitpick comments (4)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (2)
40-68: 프로퍼티 네이밍이 PascalCase를 사용하고 있습니다.Kotlin 컨벤션상 프로퍼티는 camelCase(
cherrishPink,red100등)를 권장합니다. 현재 PascalCase는 클래스/인터페이스 명명에 주로 사용됩니다.디자인 시스템 토큰명과 일치시키려는 의도라면 현재 방식도 허용 가능하지만, 코드베이스 전반의 일관성을 위해 camelCase 사용을 고려해 주세요.
101-116: 밝은 색상의 Preview가 흰 배경에서 보이지 않을 수 있습니다.Red100~Red400 등 밝은 색상들이 기본 흰색 배경에서 구분하기 어려울 수 있습니다. Gray 색상 Preview(lines 123-126)처럼 어두운 배경을 추가하거나, Preview에
showBackground = true와backgroundColor를 지정하는 것을 고려해 주세요.app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (2)
22-46: 프로퍼티 네이밍이 snake_case를 사용하고 있습니다.Kotlin 컨벤션상 프로퍼티는 camelCase(
headlineB20,title1Sb18등)를 권장합니다. 현재headline_b_20형식은 디자인 시스템 문서와 일치시키려는 의도로 보이므로, 팀 내 합의가 있다면 유지해도 무방합니다.
16-19: FontFamily를 단일 Font로 생성하고 있습니다.현재 각 weight별로 별도의
FontFamily를 만들고 있습니다. 일반적으로 하나의FontFamily에 여러 weight의Font를 등록하는 방식이 더 유연합니다. 현재 방식도 동작하지만, 향후 weight 변경이 필요한 경우 확장성이 제한될 수 있습니다.🔎 대안: 단일 FontFamily에 여러 weight 등록
val PretendardFontFamily = FontFamily( Font(R.font.pretendard_regular, FontWeight.Normal), Font(R.font.pretendard_medium, FontWeight.Medium), Font(R.font.pretendard_semibold, FontWeight.SemiBold), Font(R.font.pretendard_bold, FontWeight.Bold), ) // TextStyle에서 fontWeight로 구분 val headline_b_20 = TextStyle( fontFamily = PretendardFontFamily, fontWeight = FontWeight.Bold, fontSize = 20.sp, letterSpacing = 0.01.em, )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
app/src/main/res/font/pretendard_bold.ttfis excluded by!**/*.ttfapp/src/main/res/font/pretendard_medium.ttfis excluded by!**/*.ttfapp/src/main/res/font/pretendard_regular.ttfis excluded by!**/*.ttfapp/src/main/res/font/pretendard_semibold.ttfis excluded by!**/*.ttf
📒 Files selected for processing (3)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: - Jetpack Compose 구조, 상태 관리, recomposition 최적화에 집중
- ViewModel, UiState, 단방향 데이터 흐름 패턴 검토
- 불필요한 recomposition 가능성, remember/derivedStateOf 적절한 사용 확인
- 네이밍 컨벤션, 가독성, Google 권장 Android 아키텍처 준수 여부
Files:
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt
🧬 Code graph analysis (2)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
CherrishTheme(40-64)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
CherrishTheme(40-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: PR Lint Check
- GitHub Check: PR Build Check
🔇 Additional comments (6)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
14-25: CompositionLocal 기반 테마 구조가 잘 구현되어 있습니다.
CherrishThemeobject가@ReadOnlyComposable을 통해 colors와 typography에 효율적으로 접근하도록 설계되었습니다. Google의 권장 커스텀 테마 패턴을 잘 따르고 있습니다.app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (2)
13-22:CherrishPink과Red800이 동일한 색상 값입니다.두 색상 모두
0xFFEB445B로 정의되어 있습니다. PR 설명에서 Figma에서의 중복을 언급하셨는데, 이대로 유지하시려는 의도가 맞는지 확인 부탁드립니다.동일한 값이 의도적이라면 문제없지만, 이후 유지보수 시 혼란을 줄 수 있으므로 주석으로 의도를 명시하거나 하나의 토큰만 사용하는 것을 권장합니다.
99-99:staticCompositionLocalOf사용이 적절합니다.색상 값은 런타임에 자주 변경되지 않으므로
staticCompositionLocalOf가compositionLocalOf보다 성능상 유리합니다. 불필요한 recomposition을 방지하는 좋은 선택입니다.app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (3)
77-125:title2,body,caption스타일에letterSpacing이 누락되었습니다.
headline과title1스타일에는letterSpacing = 0.01.em이 설정되어 있지만,title2(line 77~),body(line 92~),caption(line 122~) 스타일에는 없습니다.디자인 스펙에 따라 의도적인 것인지 확인해 주세요. 일관성을 위해 모든 스타일에 letterSpacing을 명시하거나, 생략된 이유를 주석으로 남기는 것을 권장합니다.
129-129:staticCompositionLocalOf사용이 적절합니다.Typography도 런타임에 자주 변경되지 않으므로 Color와 마찬가지로
staticCompositionLocalOf가 적합합니다.
131-219: Preview가 모든 Typography 스타일을 잘 보여주고 있습니다.
CherrishTheme으로 감싸서 실제 사용 환경과 동일하게 테스트할 수 있어 좋습니다. 디자인 시스템 구현 확인에 유용합니다.
| val view = LocalView.current | ||
| if (!view.isInEditMode) { | ||
| SideEffect { | ||
| (view.context as Activity).window.run { | ||
| WindowCompat | ||
| .getInsetsController(this, view) | ||
| .isAppearanceLightStatusBars = true | ||
| } | ||
| } |
There was a problem hiding this comment.
view.context as Activity 캐스트가 안전하지 않습니다.
view.context가 항상 Activity가 아닐 수 있습니다. ContextWrapper로 래핑되어 있을 경우 ClassCastException이 발생할 수 있습니다.
🔎 안전한 Activity 참조 방법
- SideEffect {
- (view.context as Activity).window.run {
- WindowCompat
- .getInsetsController(this, view)
- .isAppearanceLightStatusBars = true
- }
- }
+ SideEffect {
+ val window = (view.context as? Activity)?.window ?: return@SideEffect
+ WindowCompat
+ .getInsetsController(window, view)
+ .isAppearanceLightStatusBars = true
+ }🤖 Prompt for AI Agents
In @app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt
around lines 48 - 56, The cast "(view.context as Activity)" is unsafe; replace
it with a safe lookup that unwraps ContextWrappers to find an Activity before
using it (e.g., from LocalView.current get view, guard !view.isInEditMode, then
in SideEffect attempt to resolve Activity by walking context wrappers and only
call activity.window.run { WindowCompat.getInsetsController(this,
view).isAppearanceLightStatusBars = true } if an Activity was found); ensure you
handle the null case by skipping the window/insets call rather than casting
directly.
| @Composable | ||
| private fun CherrishColors.toMaterialColorScheme(): ColorScheme { | ||
| return lightColorScheme( | ||
| primary = CherrishTheme.colors.CherrishPink, | ||
| onPrimary = CherrishTheme.colors.Gray0, | ||
|
|
||
| secondary = CherrishTheme.colors.Red700, | ||
| onSecondary = CherrishTheme.colors.Gray0, | ||
|
|
||
| background = CherrishTheme.colors.Gray0, | ||
| onBackground = CherrishTheme.colors.Gray900, | ||
|
|
||
| surface = CherrishTheme.colors.Gray900, | ||
| onSurface = CherrishTheme.colors.Gray0, | ||
|
|
||
| error = CherrishTheme.colors.Red700, | ||
| onError = CherrishTheme.colors.Gray0, | ||
| ) |
There was a problem hiding this comment.
확장 함수에서 this 대신 CherrishTheme.colors를 사용하고 있습니다.
CherrishColors의 확장 함수인데 receiver(this)를 사용하지 않고 CherrishTheme.colors를 참조합니다. 이 경우 함수에 전달된 colors 인스턴스가 무시되어 항상 현재 CompositionLocal의 colors가 사용됩니다.
🔎 receiver를 사용하도록 수정
@Composable
private fun CherrishColors.toMaterialColorScheme(): ColorScheme {
return lightColorScheme(
- primary = CherrishTheme.colors.CherrishPink,
- onPrimary = CherrishTheme.colors.Gray0,
+ primary = this.CherrishPink,
+ onPrimary = this.Gray0,
- secondary = CherrishTheme.colors.Red700,
- onSecondary = CherrishTheme.colors.Gray0,
+ secondary = this.Red700,
+ onSecondary = this.Gray0,
- background = CherrishTheme.colors.Gray0,
- onBackground = CherrishTheme.colors.Gray900,
+ background = this.Gray0,
+ onBackground = this.Gray900,
- surface = CherrishTheme.colors.Gray900,
- onSurface = CherrishTheme.colors.Gray0,
+ surface = this.Gray900,
+ onSurface = this.Gray0,
- error = CherrishTheme.colors.Red700,
- onError = CherrishTheme.colors.Gray0,
+ error = this.Red700,
+ onError = this.Gray0,
)
}또는 확장 함수가 아닌 일반 함수로 변경하는 것도 방법입니다.
🤖 Prompt for AI Agents
In @app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt
around lines 66 - 83, The extension function toMaterialColorScheme currently
ignores its receiver and directly references CherrishTheme.colors; update the
body of toMaterialColorScheme to use the receiver (e.g., replace
CherrishTheme.colors.CherrishPink, .Gray0, .Red700, etc. with this.CherrishPink
/ this.Gray0 / this.Red700 or simply CherrishPink when unambiguous) so the
passed CherrishColors instance is used, or alternatively convert the extension
into a regular function that accepts a CherrishColors parameter if you intend to
always use the global theme.
There was a problem hiding this comment.
P1 : 래빗이 말이맞아요 !!
현재 코드는 CherrishColors의 확장 함수인데도 불구하고 CherrishTheme.colors를 통해 접근하고 있어요.
이는 아래와 같은 문제가 발생할 수 있어요 !
- 순환 참조 위험:
CherrishTheme.colors는 실행 시점에CompositionLocal에서 가져오는 값인데, 이 함수가 바로 그 색상을 설정하는 함수입니다. - 확장 함수의 의미 상실:
CherrishColors를 확장했다면 해당 인스턴스(this)를 사용해야 합니다.
primary = this.CherrishPink, 이런식으로 사용해주세요 ~~
There was a problem hiding this comment.
@nhyeonii 분석을 확인해주셔서 감사합니다! 추가로 설명해주신 순환 참조 위험성도 중요한 포인트네요.
확장 함수의 receiver(this)를 사용하면 더 명확하고 안전한 코드가 될 것 같습니다. PR 작성자분께서 제안하신 대로 수정해주시길 기대합니다! 🙂
ヽ(^‥^=ゞ)
There was a problem hiding this comment.
우와 네넵 !! 두 분 (?) 다 감사합니다 말씀하신 기반으로 리팩토링 하겠습니다 !
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
nhyeonii
left a comment
nhyeonii
left a comment
There was a problem hiding this comment.
먼저 리뷰 하기 전에 !! PR 올리기 전에 ktLintFormat 한번 돌리구 올려주세용 ~~ !!
nhyeonii
left a comment
nhyeonii
left a comment
There was a problem hiding this comment.
빠른 작업 체고인데 ~ ?!?! cherrishPink와 red800 같은 경우는 디쌤한테 질문 남겨뒀으니 지금 이 상태로 두고 나중에 수정해도 괜찮을 거 같아요 !! 리뷰 한번 확인해보시구 수정 사항 반영해주세요 ~~ 🫰🫰🫰 수고해써
| val Purple80 = Color(0xFFD0BCFF) | ||
| val PurpleGrey80 = Color(0xFFCCC2DC) | ||
| val Pink80 = Color(0xFFEFB8C8) | ||
| val CherrishPink = Color(0xFFEB445B) |
There was a problem hiding this comment.
P1 : 저희 변수명 같은 경우는 lowerCamelCase 쓰는 컨벤션 통일할거라 cherrishPink 이런식으루 컬러명들 수정 부탁해요 ~~
There was a problem hiding this comment.
앗 넵넵 !! 적용하겠습니다 :)
| fun CherrishRedColorsPreview() { | ||
| CherrishTheme { | ||
| Column { | ||
| Text("Red100", style = CherrishTheme.typography.body1_m_14, color = CherrishTheme.colors.Red100) |
There was a problem hiding this comment.
P2 : 사소한거이긴 하지만 파라미터가 여러개일 경우에는 파라미터명을 전부 표시해주는게 좋습니다 ㅎㅎ 텍스트로 text = "Red100" 이런식으루 !
| @Immutable | ||
| data class CherrishTypography( | ||
|
|
||
| val headline_b_20: TextStyle, |
There was a problem hiding this comment.
P1 : 여기 텍스트 관련 변수명들도 lowerCamelCase로 수정 부탁해요 !! headlineB20 이런식으로 갑시다 ㅎㅎ
| CherrishTheme { | ||
| Column { | ||
| Text( | ||
| "headline_b_20", |
There was a problem hiding this comment.
P2 : 여기두 text = "headlineB20" 이렇게 파라미터명 써줍시다 ㅎㅎ
|
|
||
| @Preview | ||
| @Composable | ||
| fun CherrishTypographyPreview() { |
There was a problem hiding this comment.
P1 : 프리뷰에는 꼭 private 접근 제한자 붙여주세요 !
|
|
||
| @Preview | ||
| @Composable | ||
| fun CherrishRedColorsPreview() { |
| typography = Typography, | ||
| content = content | ||
| @Composable | ||
| private fun CherrishColors.toMaterialColorScheme(): ColorScheme { |
There was a problem hiding this comment.
P3 : 현재 toMaterialColorScheme()를 통해 Material3 ColorScheme으로 변환하고 있는데, 저희 프로젝트에서는 대부분의 컴포넌트를 커스텀으로 구현하고 색상도 CherrishTheme.colors.xxx 형태로 명시적으로 사용할 예정입니다 ! 그래서 제가 봤을 땐 이 매핑 함수가 불필요할 수도 있을 거라고 생각해요. 이에 대한 혜민님의 의견이 궁금합니다 !
There was a problem hiding this comment.
매핑을 하지 않고 Theme을 호출하니까 오류가 뜨더라고요 😭 그래서 위에 처럼 코드를 구현했습니다 !
다시 한 번 말씀하신대로 구현해보겠습니다 !
There was a problem hiding this comment.
매핑을 하지 않고 theme를 호출한다는게 어떤 의미일까용 ...?!? 질문이 이해가 안대요...ㅜㅜ
There was a problem hiding this comment.
적힌 매핑 함수 없이, MaterialTheme에 적용을 하려고 하는데 ColorScheme 구조와 맞지 않아 계속 오류가 뜨고 컬러를 읽지 못하더라고요 😭
그래서 해결하기 위해 컬러 매핑 함수를 구현했습니다.
| content = content | ||
| @Composable | ||
| private fun CherrishColors.toMaterialColorScheme(): ColorScheme { | ||
| return lightColorScheme( |
There was a problem hiding this comment.
P1 : 현재 저희는 다크모드를 지원하지 않는데 lightColorScheme()를 사용하고 있어 코드를 볼 때 혼란을 줄 수 있을 것 같아요. 만약 이 material 변환 함수를 계속 가지고 가게 된다면 lightColorScheme보다는 colorScheme 같은 표현이 더 나을 것 같은데 어떠신가용 ?!
There was a problem hiding this comment.
오 좋은것 같아요 ! 참고하다가 다크모드까지 참고해버렸네요 ,, ㅎㅎ 수정하겠습니다 !!
| @Composable | ||
| private fun CherrishColors.toMaterialColorScheme(): ColorScheme { | ||
| return lightColorScheme( | ||
| primary = CherrishTheme.colors.CherrishPink, | ||
| onPrimary = CherrishTheme.colors.Gray0, | ||
|
|
||
| secondary = CherrishTheme.colors.Red700, | ||
| onSecondary = CherrishTheme.colors.Gray0, | ||
|
|
||
| background = CherrishTheme.colors.Gray0, | ||
| onBackground = CherrishTheme.colors.Gray900, | ||
|
|
||
| surface = CherrishTheme.colors.Gray900, | ||
| onSurface = CherrishTheme.colors.Gray0, | ||
|
|
||
| error = CherrishTheme.colors.Red700, | ||
| onError = CherrishTheme.colors.Gray0, | ||
| ) |
There was a problem hiding this comment.
P1 : 래빗이 말이맞아요 !!
현재 코드는 CherrishColors의 확장 함수인데도 불구하고 CherrishTheme.colors를 통해 접근하고 있어요.
이는 아래와 같은 문제가 발생할 수 있어요 !
- 순환 참조 위험:
CherrishTheme.colors는 실행 시점에CompositionLocal에서 가져오는 값인데, 이 함수가 바로 그 색상을 설정하는 함수입니다. - 확장 함수의 의미 상실:
CherrishColors를 확장했다면 해당 인스턴스(this)를 사용해야 합니다.
primary = this.CherrishPink, 이런식으로 사용해주세요 ~~
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @hyeminililo. * #4 (comment) The following files were modified: * `app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt` * `app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt` * `app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt`
usuuhyn
left a comment
usuuhyn
left a comment
There was a problem hiding this comment.
수고하셨습니다! 래빗이와 리드님이 이미 코멘트 잘 달아주셔서 달 게 없네요...ㅎㅎ
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt:
- Line 152: Both preview functions are missing the private modifier; make
CherrishGrayColorsPreview and CherrishGreenColorsPreview private so they are not
exposed as part of the public API. Locate the two function declarations (symbols
CherrishGrayColorsPreview and CherrishGreenColorsPreview) and add the private
visibility modifier to each declaration.
🧹 Nitpick comments (2)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (1)
16-19: 폰트 패밀리 변수들을 private로 변경하세요.디자인 토큰의 캡슐화를 위해 이 변수들을
private으로 선언하는 것이 좋습니다. 사용자는CherrishTheme.typography를 통해 접근해야 하며, 원시 토큰에 직접 접근할 필요가 없습니다.♻️ 제안하는 수정
-val cherrishFontBold = FontFamily(Font(R.font.pretendard_bold)) -val cherrishFontSemiBold = FontFamily(Font(R.font.pretendard_semibold)) -val cherrishFontMedium = FontFamily(Font(R.font.pretendard_medium)) -val cherrishFontRegular = FontFamily(Font(R.font.pretendard_regular)) +private val cherrishFontBold = FontFamily(Font(R.font.pretendard_bold)) +private val cherrishFontSemiBold = FontFamily(Font(R.font.pretendard_semibold)) +private val cherrishFontMedium = FontFamily(Font(R.font.pretendard_medium)) +private val cherrishFontRegular = FontFamily(Font(R.font.pretendard_regular))app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (1)
13-38: 색상 토큰 변수들을 private로 변경하세요.디자인 토큰 캡슐화를 위해 원시 색상 값들을
private으로 선언하는 것이 좋습니다. 사용자는CherrishTheme.colors를 통해 접근해야 하며, 개별 색상 상수에 직접 접근할 필요가 없습니다.♻️ 제안하는 수정
-val cherrishPink = Color(0xFFEB445B) +private val cherrishPink = Color(0xFFEB445B) -val red100 = Color(0xFFFEF7F8) -val red200 = Color(0xFFFFF4F7) -val red300 = Color(0xFFFFE4EB) -val red400 = Color(0xFFFFD3DC) -val red500 = Color(0xFFFFADBD) -val red600 = Color(0xFFFF879C) -val red700 = Color(0xFFFF617B) -val red800 = Color(0xFFFF4A63) +private val red100 = Color(0xFFFEF7F8) +private val red200 = Color(0xFFFFF4F7) +private val red300 = Color(0xFFFFE4EB) +private val red400 = Color(0xFFFFD3DC) +private val red500 = Color(0xFFFFADBD) +private val red600 = Color(0xFFFF879C) +private val red700 = Color(0xFFFF617B) +private val red800 = Color(0xFFFF4A63) -val gray0 = Color(0xFFFFFFFF) -val gray100 = Color(0xFFFCFCFC) -val gray200 = Color(0xFFF7F8F9) -val gray300 = Color(0xFFEFF1F3) -val gray400 = Color(0xFF9FA4A9) -val gray500 = Color(0xFFCACDD1) -val gray600 = Color(0xFF9FA4A9) -val gray700 = Color(0xFF73787E) -val gray800 = Color(0xFF464C52) -val gray900 = Color(0xFF26282B) -val gray1000 = Color(0xFF1B1D1F) +private val gray0 = Color(0xFFFFFFFF) +private val gray100 = Color(0xFFFCFCFC) +private val gray200 = Color(0xFFF7F8F9) +private val gray300 = Color(0xFFEFF1F3) +private val gray400 = Color(0xFF9FA4A9) +private val gray500 = Color(0xFFCACDD1) +private val gray600 = Color(0xFF9FA4A9) +private val gray700 = Color(0xFF73787E) +private val gray800 = Color(0xFF464C52) +private val gray900 = Color(0xFF26282B) +private val gray1000 = Color(0xFF1B1D1F) -val green1 = Color(0xFFFBFFF3) -val green2 = Color(0xFFBEDCB8) -val green3 = Color(0xFF9AD342) +private val green1 = Color(0xFFFBFFF3) +private val green2 = Color(0xFFBEDCB8) +private val green3 = Color(0xFF9AD342)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: - Jetpack Compose 구조, 상태 관리, recomposition 최적화에 집중
- ViewModel, UiState, 단방향 데이터 흐름 패턴 검토
- 불필요한 recomposition 가능성, remember/derivedStateOf 적절한 사용 확인
- 네이밍 컨벤션, 가독성, Google 권장 Android 아키텍처 준수 여부
Files:
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt
🧬 Code graph analysis (2)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
CherrishTheme(38-61)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
CherrishTheme(38-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: PR Build Check
- GitHub Check: PR Lint Check
🔇 Additional comments (7)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (4)
21-46: LGTM!
@Immutable어노테이션과 data class 구조가 올바르게 적용되었습니다. 불필요한 recomposition을 방지하고 타이포그래피 시스템이 명확하게 정의되어 있습니다.
48-127: letterSpacing과 lineHeight 일관성을 확인하세요.타이포그래피 정의에서 다음 사항들을 확인해주세요:
- letterSpacing 불일관성: headline과 title1 스타일은
letterSpacing = 0.01.em을 명시하지만, title2, body, caption 스타일들은 생략되어 있습니다.- lineHeight 누락: 모든 스타일에
lineHeight가 명시되지 않아 다중 라인 텍스트 렌더링 시 일관성이 떨어질 수 있습니다.디자인 시스템 명세(Figma)와 비교하여 이 값들이 의도적으로 생략된 것인지 확인하고, 필요시 추가해주세요.
129-129: LGTM!
staticCompositionLocalOf사용이 적절합니다. 디자인 토큰은 런타임에 변경되지 않으므로 불필요한 recomposition 오버헤드를 방지합니다.
131-219: LGTM!Preview 함수가 올바르게 구현되었습니다.
private접근 제한자가 적용되었고, 모든 파라미터명이 명시되어 있으며, 14개 타이포그래피 스타일을 모두 시각화하고 있습니다.app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (3)
40-68: LGTM!
@Immutable어노테이션과 data class 구조가 올바르게 적용되었습니다. 색상 시스템이 명확하게 정의되어 있으며 Compose recomposition 최적화에 적합합니다.
70-97: LGTM!기본 색상 인스턴스가 올바르게 초기화되었으며, 모든 파라미터명이 명시되어 가독성이 높습니다.
99-99: LGTM!
staticCompositionLocalOf사용이 적절합니다. Type.kt와 일관된 패턴을 사용하고 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt:
- Around line 26-28: gray400 and gray600 in Color.kt are both defined as
Color(0xFF9FA4A9) which is incorrect; update gray600 to the darker hex value
from the Figma design system so each gray step has a unique luminance (change
the definition of gray600 to the correct hex from Figma, keeping gray400 as
Color(0xFF9FA4A9) and ensuring the symbol name gray600 reflects the darker
color).
- Line 148: The preview function CherrishGrayColorsPreview is currently public
but should not be part of the public API; make it private by adding the private
modifier to the CherrishGrayColorsPreview function declaration so the preview
remains internal to the module and not exposed as a public API surface.
- Line 214: The Preview function CherrishGreenColorsPreview() should not be
public; mark it private to prevent it from becoming part of the public API.
Locate the CherrishGreenColorsPreview() function and add the private modifier
(e.g., make it a private fun) so the preview is scoped to the file and not
exposed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: - Jetpack Compose 구조, 상태 관리, recomposition 최적화에 집중
- ViewModel, UiState, 단방향 데이터 흐름 패턴 검토
- 불필요한 recomposition 가능성, remember/derivedStateOf 적절한 사용 확인
- 네이밍 컨벤션, 가독성, Google 권장 Android 아키텍처 준수 여부
Files:
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt
🧬 Code graph analysis (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
CherrishTheme(38-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: PR Build Check
- GitHub Check: PR Lint Check
🔇 Additional comments (4)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (4)
38-65: 잘 구현된 색상 데이터 클래스입니다.
@Immutable어노테이션과 불변 프로퍼티 사용으로 Compose의 recomposition 최적화에 적합합니다.
67-93: 기본 색상 인스턴스가 올바르게 구성되어 있습니다.모든 파라미터에 명시적 이름을 사용하여 가독성이 우수합니다.
95-95: CompositionLocal 구현이 적절합니다.색상 테마는 런타임에 변경되지 않으므로
staticCompositionLocalOf사용이 올바른 선택입니다.
97-144: Red 색상 프리뷰가 잘 구현되어 있습니다.
private접근 제한자가 올바르게 적용되었고, 모든 파라미터에 명시적 이름을 사용하여 코드 가독성이 우수합니다.
| val gray400 = Color(0xFF9FA4A9) | ||
| val gray500 = Color(0xFFCACDD1) | ||
| val gray600 = Color(0xFF9FA4A9) |
There was a problem hiding this comment.
gray400와 gray600이 동일한 색상값을 가지고 있습니다.
두 색상 모두 Color(0xFF9FA4A9)로 정의되어 있습니다. 일반적으로 그레이스케일 팔레트에서 각 단계는 고유한 명도를 가져야 합니다. Figma 디자인 시스템의 실제 색상값을 확인하여 수정해주세요.
🎨 예상되는 수정 방향
gray600은 gray400보다 어두운 색상이어야 합니다. Figma에서 올바른 값을 확인 후:
val gray400 = Color(0xFF9FA4A9)
val gray500 = Color(0xFFCACDD1)
-val gray600 = Color(0xFF9FA4A9)
+val gray600 = Color(0xFFXXXXXXXX) // Figma에서 실제 gray600 값으로 변경
val gray700 = Color(0xFF73787E)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt
around lines 26 - 28, gray400 and gray600 in Color.kt are both defined as
Color(0xFF9FA4A9) which is incorrect; update gray600 to the darker hex value
from the Figma design system so each gray step has a unique luminance (change
the definition of gray600 to the correct hex from Figma, keeping gray400 as
Color(0xFF9FA4A9) and ensuring the symbol name gray600 reflects the darker
color).
nhyeonii
left a comment
nhyeonii
left a comment
There was a problem hiding this comment.
리뷰 반영 부탁드려요이 ! 거의 다 햇다ㅏㅏ
| val gray100 = Color(0xFFFCFCFC) | ||
| val gray200 = Color(0xFFF7F8F9) | ||
| val gray300 = Color(0xFFEFF1F3) | ||
| val gray400 = Color(0xFF9FA4A9) |
There was a problem hiding this comment.
P1 : gray400 컬러 E9EBED 이거 인것 같아요 ~
| data class CherrishTypography( | ||
|
|
||
| val headlineB20: TextStyle, | ||
| val headlineSb20: TextStyle, |
There was a problem hiding this comment.
P1 : headlineSB20 이렇게 해주세요 ! SemiBold이니까 가운데는 대문자로 해주는게 좋을 것 같슴둥 밑에도 다 반영해주세요!
nhyeonii
left a comment
nhyeonii
left a comment
There was a problem hiding this comment.
깔끔하고만 ~~ !!! 고생햇더요 !!!!!!! LGTM 🚀🚀🚀🚀
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (3)
16-19: 폰트 패밀리 가시성 검토 권장현재 폰트 패밀리들이 public으로 선언되어 있습니다. 디자인 시스템의 캡슐화를 위해
internal또는private가시성 제한자를 고려해보세요. 일반적으로 외부에서는CherrishTheme.typography를 통해 접근하는 것이 권장됩니다.다만, 커스텀 TextStyle 생성 등 특수한 경우를 위해 폰트 패밀리 접근이 필요하다면 현재 상태를 유지해도 됩니다.
48-127: letterSpacing 일관성 및 lineHeight 추가 검토 필요일부 TextStyle(headline, title1)에는
letterSpacing = 0.01.em이 지정되어 있지만, title2, body, caption 스타일에는 지정되지 않았습니다. 이것이 Figma 디자인 명세를 따른 의도적인 것인지 확인해주세요.또한, 모든 TextStyle에
lineHeight가 누락되어 있습니다. lineHeight는 텍스트의 수직 리듬과 다중 라인 텍스트 렌더링에 중요한 속성입니다. 디자인 명세에 lineHeight 값이 정의되어 있다면 추가하는 것을 권장합니다.예시:
headlineB20 = TextStyle( fontFamily = cherrishFontBold, fontSize = 20.sp, lineHeight = 28.sp, // 예시 값 letterSpacing = 0.01.em )
131-219: Preview 스크롤 기능 추가 고려Preview가 모든 타이포그래피 변형을 잘 보여주고 있으며, 이전 리뷰 피드백(private 접근 제한자, 파라미터명)이 모두 반영되었습니다.
다만, Column에 스크롤 기능이 없어 Android Studio에서 작은 프리뷰 창에서는 일부 타이포그래피가 잘릴 수 있습니다. 개발자 경험 개선을 위해
Modifier.verticalScroll(rememberScrollState())를 추가하는 것을 고려해보세요.♻️ 선택적 개선 제안
+import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.verticalScroll +import androidx.compose.ui.Modifier + @Preview @Composable private fun CherrishTypographyPreview() { CherrishTheme { - Column { + Column(modifier = Modifier.verticalScroll(rememberScrollState())) { Text(app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (2)
13-36: 색상 상수의 접근 제한자를internal로 설정하는 것을 고려해보세요.현재 색상 상수들(
red100,gray0,green1등)이 public으로 선언되어 있어, 코드베이스의 어디서든 직접 접근이 가능합니다. 디자인 시스템의 일관성을 위해서는CherrishTheme.colors.red100과 같이 테마를 통한 접근을 권장하는 것이 좋습니다.
internal키워드를 추가하면 모듈 외부로의 노출을 막고, 모듈 내에서는defaultCherrishColors초기화에 사용할 수 있습니다.♻️ 제안하는 수정
-val red100 = Color(0xFFFEF7F8) -val red200 = Color(0xFFFFF4F7) +internal val red100 = Color(0xFFFEF7F8) +internal val red200 = Color(0xFFFFF4F7) // ... 나머지 색상 상수들도 동일하게 적용
95-95:staticCompositionLocalOf사용이 적절하며, 네이밍 컨벤션 개선을 고려해보세요.
staticCompositionLocalOf는 정적 색상 팔레트에 적합한 선택입니다. 다만, Compose의 일반적인 컨벤션에 따르면 CompositionLocal 이름은LocalCherrishColors처럼 "Provider" 접미사 없이 사용하는 것이 더 일반적입니다.참고: 향후 다크 모드나 동적 테마가 필요한 경우
compositionLocalOf로 변경을 고려해야 합니다.♻️ 네이밍 개선 제안
-val LocalCherrishColorsProvider = staticCompositionLocalOf { defaultCherrishColors } +val LocalCherrishColors = staticCompositionLocalOf { defaultCherrishColors }이 경우 Theme.kt에서도 해당 이름을 업데이트해야 합니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: - Jetpack Compose 구조, 상태 관리, recomposition 최적화에 집중
- ViewModel, UiState, 단방향 데이터 흐름 패턴 검토
- 불필요한 recomposition 가능성, remember/derivedStateOf 적절한 사용 확인
- 네이밍 컨벤션, 가독성, Google 권장 Android 아키텍처 준수 여부
Files:
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.ktapp/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt
🧬 Code graph analysis (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (1)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Theme.kt (1)
CherrishTheme(38-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: PR Lint Check
- GitHub Check: PR Build Check
🔇 Additional comments (5)
app/src/main/java/com/cherrish/android/core/designsystem/theme/Type.kt (2)
21-46: LGTM!
@Immutable어노테이션과 data class 사용이 적절합니다. Compose 리컴포지션 최적화에 도움이 되며, 네이밍 컨벤션(lowerCamelCase)도 이전 리뷰 피드백을 잘 반영했습니다.
129-129: LGTM!디자인 시스템 타이포그래피에
staticCompositionLocalOf사용이 적절합니다. 자주 변경되지 않는 값에 최적화되어 있으며, Theme.kt의CherrishTheme과 올바르게 통합되었습니다.app/src/main/java/com/cherrish/android/core/designsystem/theme/Color.kt (3)
38-65: LGTM!@Immutable어노테이션이 올바르게 적용되었습니다.
CherrishColors데이터 클래스에@Immutable어노테이션을 사용한 것은 Compose의 스마트 리컴포지션 최적화에 매우 적합합니다. 이를 통해 Compose는 색상 값이 변경되지 않았을 때 불필요한 리컴포지션을 건너뛸 수 있습니다.
67-93: LGTM! 명명된 매개변수를 일관되게 사용했습니다.모든 색상 프로퍼티에 명명된 인자를 사용하여
defaultCherrishColors를 초기화한 것은 팀 컨벤션을 잘 따르고 있으며, 코드의 가독성과 유지보수성을 향상시킵니다.
97-234: LGTM! 프리뷰 함수들이 잘 구현되었습니다.프리뷰 함수들이 모두
private으로 적절히 제한되었고,CherrishTheme을 통한 색상 접근 방식을 명확히 보여줍니다. 특히CherrishGrayColorsPreview에서 밝은 색상의 가시성을 위해 어두운 배경을 사용한 것은 좋은 UX 고려사항입니다.
|
|
||
| ) | ||
|
|
||
| val defaultCherrishTypography = CherrishTypography( |
There was a problem hiding this comment.
TextStyle에 lineHeightStyle이 지정되어 있지 않아서
폰트 등등에 따라 baseline이 달라져 디쌤들이 원하는 대로 나오지 않는 경우도 발생할 수 있습니다.
그래서 공통으로 lineHeightStyle을 지정해주셔야 될 것 같습니다!
아마 이거 적어주시면 될 거예요!!
lineHeightStyle = LineHeightStyle( alignment = LineHeightStyle.Alignment.Center, trim = LineHeightStyle.Trim.None )
추가로 아래 팟장님 글 참고해주시면 될 것 같습니다!!
Compose LineHeight 이슈 정리 글
4 participants
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
피그마를 바탕으로 디자인 시스템을 구현했습니다
그런데 피그마를 보니 CherrishPink와 Red800의 색상이 중복되더라고요 😭 일단 CherrishPink가 변경될 수 있을 것 같아 둘 다 구현해두긴 했는데 이와 관련하여 코드리뷰 해주시면 좋을 것 같습니다! 그리고 제가 만든 디자인 시스템은 아래처럼 사용하시면 됩니다
color = CherrishTheme.colors.Gray900style = CherrishTheme.typography.body3_m_12그리고 참고자료는 안드로이드 파트 내에 있는 합세 노션 자료를 참고했습니다 ! 편하게 코드리뷰 해주세요 🥰
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.