[FEAT/17] 디자인시스템 구축 #19
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
디자인 시스템은 따로 빼 놔도 괜찮을 것 같아! 예전에 UI 모듈 하나라 같이 넣었다가, UI 모듈이 추가된 적이 있었거든! |
| // Background | ||
| val BackgroundBase = Color(0xFFF7F8FA) | ||
| val BackgroundSurface = Color(0xFFFFFFFF) | ||
| val BackgroundElevated = Color(0xFFFFFFFF) | ||
| val BackgroundOnSurface = Color(0xFFF7F8FA) | ||
| val BackgroundOnElevated = Color(0xFFF7F8FA) | ||
| val BackgroundSurfaceInverse = Color(0xFF1B1D1F) | ||
| val BackgroundDimmed = Color(0xB3000000) |
There was a problem hiding this comment.
시멘틱 컬러에 사용하는 Color 객체는 하드코딩 대신 피그마와 동일하게 컬러 팔레트를 먼저 구성한 뒤 사용하면 좋을 것 같아! 아래는 예시 코드!
| // Background | |
| val BackgroundBase = Color(0xFFF7F8FA) | |
| val BackgroundSurface = Color(0xFFFFFFFF) | |
| val BackgroundElevated = Color(0xFFFFFFFF) | |
| val BackgroundOnSurface = Color(0xFFF7F8FA) | |
| val BackgroundOnElevated = Color(0xFFF7F8FA) | |
| val BackgroundSurfaceInverse = Color(0xFF1B1D1F) | |
| val BackgroundDimmed = Color(0xB3000000) | |
| // 팔레트 | |
| val White = Color(0xFFFFFFFF) | |
| val Gray10 = Color(0xFFFAFBFC) | |
| val Gray30 = Color(0xFFF7F8FA) | |
| val Gray50 = Color(0xFFF2F4F7) | |
| val Gray100 = Color(0xFFD7DCE0) | |
| // ... | |
| val Gray900 = Color(0xFF1B1D1F) | |
| val Black = Color(0xFF000000) | |
| // Background | |
| val BackgroundBase = Gray30 | |
| val BackgroundSurface = White | |
| val BackgroundElevated = White | |
| val BackgroundOnSurface = Gray30 | |
| val BackgroundOnElevated = Gray30 | |
| val BackgroundSurfaceInverse = Gray900 | |
| val BackgroundDimmed = Black.copy(alpha = 0.7F) |
There was a problem hiding this comment.
오 왜 이 방법을 생각 못했을까..!
이렇게 하는 게 확실히 좋은 것 같네용 반영하겠습니다~
| modifier = modifier, | ||
| style = KnownKnownsTheme.typography.body13Bold, | ||
| color = KnownKnownsTheme.colors.fillWhite |
| @Composable | ||
| fun KnownKnownsTypography(): KnownKnownsTypography = |
There was a problem hiding this comment.
Composable 이어도 return 하는 함수는 카멜케이스가 컨벤션인가봐여~ 그런데 @composable이 KnownKnownsTypography에는 붙어있는데, KnownKnownsColors에는 안 붙어있는 거 같아!
아래는 비슷한 역할의 라이브러리 코드인데, 어노테이션 안 붙어 있는 거 보면 없어도 되는 거 같기도!
There was a problem hiding this comment.
예리한 개발자..! 저도 그 예리함 본받겠습니다.
| ) | ||
|
|
||
| fun update(other: KnownKnownsColors) { |
There was a problem hiding this comment.
혹시 update 메서드를 구현한 목적이 있을까??
|
오늘 디스코드로 얘기한 내용 정리(?) 공유 드립니다.
일반 객체 대신 mutableStateOf 로 구현한 이유는 무엇일까? |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Presentation
participant CoreTheme
App->>Presentation: Launch MainActivity
Presentation->>CoreTheme: Import KnownKnownsTheme
Presentation->>CoreTheme: Access KnownKnownsTheme.colors, typography
CoreTheme-->>Presentation: Provide color and typography data
Presentation->>Presentation: Use theme data in UI composables
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
presentation/build.gradle.kts (1)
10-15:compileSdk = 36will break the build – latest released SDK is 34AGP 8.5 can compile only up to the latest public SDK (34 at the time of writing).
Using an unreleased API level causes Gradle sync failure on every workstation/CI run.- compileSdk = 36 + compileSdk = 34Keep
targetSdkin the library manifest aligned as well (or rely on the Gradlenamespace).
🧹 Nitpick comments (5)
.gitignore (1)
21-24:.kotlin/is niche; confirm it’s really generated in your CI/build flowThe IntelliJ / Gradle Kotlin plugin occasionally writes incremental-compilation caches to
.kotlin/, but this folder is not present for all setups.
If it never appears in your local/CI builds, the ignore rule is unnecessary noise.No action required if the folder is indeed produced, otherwise consider dropping it.
settings.gradle.kts (1)
24-25: Include order nitpickPlacing
:corenext to:presentation(or alphabetically) keeps the list tidy when the project scales, but this is optional.core/src/main/AndroidManifest.xml (1)
1-4: Add explicitpackageattribute for clarityWhile AGP will derive the package from
namespaceinbuild.gradle, an explicitpackageaids tools that still parse the manifest directly and avoids Lint warnings.-<manifest xmlns:android="http://schemas.android.com/apk/res/android"> +<manifest xmlns:android="http://schemas.android.com/apk/res/android" + package="com.nexters.knownknowns.core">core/src/main/java/com/nexters/knownknowns/core/theme/Theme.kt (1)
9-15: Consider consistent visibility for composition locals.There's an inconsistency in visibility:
LocalKnownKnownsColorsis public whileLocalKnownKnownsTypographyis private. Consider making both private for consistency unless there's a specific reason for the difference.-val LocalKnownKnownsColors = staticCompositionLocalOf<KnownKnownsColors> { +private val LocalKnownKnownsColors = staticCompositionLocalOf<KnownKnownsColors> {core/src/main/java/com/nexters/knownknowns/core/theme/Type.kt (1)
13-18: Consider restrictingpretendardFamilyto internal visibility
Exposing the font family publicly leaks implementation details of the design system. If external use is not required, mark itinternalorprivate.-val pretendardFamily = FontFamily( +internal val pretendardFamily = FontFamily(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
core/src/main/res/font/pretendard_bold.otfis excluded by!**/*.otfcore/src/main/res/font/pretendard_medium.otfis excluded by!**/*.otfcore/src/main/res/font/pretendard_regular.otfis excluded by!**/*.otfcore/src/main/res/font/pretendard_semibold.otfis excluded by!**/*.otf
📒 Files selected for processing (14)
.gitignore(1 hunks)core/.gitignore(1 hunks)core/build.gradle.kts(1 hunks)core/proguard-rules.pro(1 hunks)core/src/main/AndroidManifest.xml(1 hunks)core/src/main/java/com/nexters/knownknowns/core/theme/Color.kt(1 hunks)core/src/main/java/com/nexters/knownknowns/core/theme/Theme.kt(1 hunks)core/src/main/java/com/nexters/knownknowns/core/theme/Type.kt(1 hunks)presentation/build.gradle.kts(1 hunks)presentation/src/main/java/com/nexters/knownknowns/presentation/MainActivity.kt(3 hunks)presentation/src/main/java/com/nexters/knownknowns/presentation/ui/theme/Color.kt(0 hunks)presentation/src/main/java/com/nexters/knownknowns/presentation/ui/theme/Theme.kt(0 hunks)presentation/src/main/java/com/nexters/knownknowns/presentation/ui/theme/Type.kt(0 hunks)settings.gradle.kts(1 hunks)
💤 Files with no reviewable changes (3)
- presentation/src/main/java/com/nexters/knownknowns/presentation/ui/theme/Type.kt
- presentation/src/main/java/com/nexters/knownknowns/presentation/ui/theme/Theme.kt
- presentation/src/main/java/com/nexters/knownknowns/presentation/ui/theme/Color.kt
🔇 Additional comments (10)
core/.gitignore (1)
1-1: LGTM – standard patternIgnoring the module-local
/builddirectory is correct for an Android library.presentation/build.gradle.kts (1)
41-46: Dependency graph OKAdding
implementation(project(":core"))correctly wires the new module.core/proguard-rules.pro (1)
1-21: LGTM! Standard ProGuard template is appropriate.This is a well-structured ProGuard rules template file. Since the core module contains UI theming components, keeping all rules commented out as examples is appropriate for now. Rules can be added later if specific obfuscation or preservation needs arise.
core/build.gradle.kts (2)
1-41: Well-structured build configuration for core module.The Gradle configuration is appropriate for a Compose-based Android library module. The dependency setup using Compose BOM and bundles follows best practices for version management.
15-15: consumerProguardFiles reference is validThe
consumer-rules.profile does exist atcore/consumer-rules.pro, so no changes are needed to the Gradle configuration.presentation/src/main/java/com/nexters/knownknowns/presentation/MainActivity.kt (2)
16-16: Excellent migration to core theme module.The import change correctly references the new centralized theme from the core module, supporting the design system modularization.
37-44: Great example of design system usage.The implementation demonstrates excellent usage of the new design system:
- Proper typography usage with
.copy(fontWeight = FontWeight.Bold)following the approach discussed in PR comments- Semantic color usage with
statePositivePrimary- Clean structure with Column layout
This serves as a good reference for other developers on how to use the design system.
core/src/main/java/com/nexters/knownknowns/core/theme/Color.kt (2)
9-55: Excellent color palette structure.The private
ColorPaletteobject provides good encapsulation of base colors with a comprehensive range of grays, reds, and greens. The color values are well-organized and properly formatted.
61-115: Well-designed semantic color system.The
KnownKnownsColorsdata class follows design system best practices:
- Semantic naming for different use cases (background, fill, border, text, etc.)
- Proper use of alpha transparency for dimmed backgrounds
- Comprehensive coverage of UI states
- Data class approach enables easy copying and modification as discussed in PR comments
This provides a solid foundation for consistent color usage across the app.
core/src/main/java/com/nexters/knownknowns/core/theme/Theme.kt (1)
17-39: Excellent Compose theme implementation.The theme setup follows Compose best practices:
- Proper use of
staticCompositionLocalOffor theme dataReadOnlyComposableannotations for theme accessors- Clean
CompositionLocalProviderpattern- Good integration with
MaterialThemeThis implementation addresses the PR comment concerns by using immutable objects rather than
mutableStateOf, which is the correct approach for design system definitions.
| data class KnownKnownsTypography( | ||
| val head28: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 28.sp, | ||
| lineHeight = 1.36.em | ||
| ), | ||
| val head26: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 26.sp, | ||
| lineHeight = 1.35.em | ||
| ), | ||
| val head24: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 24.sp, | ||
| lineHeight = 1.34.em | ||
| ), | ||
| val head22: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 22.sp, | ||
| lineHeight = 1.4.em | ||
| ), | ||
| val head20: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 20.sp, | ||
| lineHeight = 1.4.em | ||
| ), | ||
| val body18: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 18.sp, | ||
| lineHeight = 1.45.em | ||
| ), | ||
| val body16: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 16.sp, | ||
| lineHeight = 1.5.em | ||
| ), | ||
| val body15: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 15.sp, | ||
| lineHeight = 1.47.em | ||
| ), | ||
| val body14: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 14.sp, | ||
| lineHeight = 1.43.em | ||
| ), | ||
| val body13: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 13.sp, | ||
| lineHeight = 1.38.em | ||
| ), | ||
| val caption12: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 12.sp, | ||
| lineHeight = 1.35.em | ||
| ), | ||
| val caption11: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 11.sp, | ||
| lineHeight = 1.28.em | ||
| ), | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark KnownKnownsTypography as @Immutable for Compose stability
Because this data class is provided through a CompositionLocal, flagging it as immutable avoids unnecessary recompositions.
-import androidx.compose.ui.text.TextStyle
+import androidx.compose.runtime.Immutable
+import androidx.compose.ui.text.TextStyle
...
-data class KnownKnownsTypography(
+@Immutable
+data class KnownKnownsTypography(🤖 Prompt for AI Agents
In core/src/main/java/com/nexters/knownknowns/core/theme/Type.kt around lines 20
to 82, the KnownKnownsTypography data class should be annotated with @Immutable
to improve Compose stability and prevent unnecessary recompositions when used in
a CompositionLocal. Add the @Immutable annotation above the data class
declaration to mark it as immutable.
| val head28: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 28.sp, | ||
| lineHeight = 1.36.em | ||
| ), | ||
| val head26: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 26.sp, | ||
| lineHeight = 1.35.em | ||
| ), | ||
| val head24: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 24.sp, | ||
| lineHeight = 1.34.em | ||
| ), | ||
| val head22: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 22.sp, | ||
| lineHeight = 1.4.em | ||
| ), | ||
| val head20: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 20.sp, | ||
| lineHeight = 1.4.em | ||
| ), |
There was a problem hiding this comment.
1.36.em (and friends) do not compile – use Float literals
em and sp extension properties are defined only for Int and Float.
Every literal such as 1.36.em, 1.35.em, 1.34.em, … is a Double and therefore fails to resolve the extension.
- lineHeight = 1.36.em
+ lineHeight = 1.36f.emRepeat the f suffix (or use 1.36f) for all occurrences: 29, 34, 39, 44, 49, 54, 59, 64, 69, 74, 79.
Without this change the module will not compile.
🤖 Prompt for AI Agents
In core/src/main/java/com/nexters/knownknowns/core/theme/Type.kt between lines
21 and 45, the lineHeight values use double literals like 1.36.em which do not
compile because the em extension is defined only for Float. Change all these
literals to Float by adding the 'f' suffix, for example, replace 1.36.em with
1.36f.em for all occurrences in the specified lines and similarly in the other
mentioned lines to ensure the code compiles correctly.
| val body16: TextStyle = createTextStyle( | ||
| fontFamily = pretendardFamily, | ||
| fontSize = 16.sp, | ||
| lineHeight = 1.5.em | ||
| ), | ||
| val body15: TextStyle = createTextStyle( |
There was a problem hiding this comment.
Example in MainActivity references body16Bold – not defined here
The PR description shows KnownKnownsTheme.typography.body16Bold, but only body16 (normal weight) exists. Either:
- Add explicit bold variants, or
- Change sample usage to
body16.copy(fontWeight = FontWeight.Bold).
Suggested addition (one example, repeat as needed):
val body16: TextStyle = createTextStyle(
fontFamily = pretendardFamily,
fontSize = 16.sp,
lineHeight = 1.5f.em
+ ),
+ val body16Bold: TextStyle = createTextStyle(
+ fontFamily = pretendardFamily,
+ fontSize = 16.sp,
+ lineHeight = 1.5f.em,
+ fontWeight = FontWeight.Bold
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val body16: TextStyle = createTextStyle( | |
| fontFamily = pretendardFamily, | |
| fontSize = 16.sp, | |
| lineHeight = 1.5.em | |
| ), | |
| val body15: TextStyle = createTextStyle( | |
| val body16: TextStyle = createTextStyle( | |
| fontFamily = pretendardFamily, | |
| fontSize = 16.sp, | |
| lineHeight = 1.5f.em | |
| ), | |
| val body16Bold: TextStyle = createTextStyle( | |
| fontFamily = pretendardFamily, | |
| fontSize = 16.sp, | |
| lineHeight = 1.5f.em, | |
| fontWeight = FontWeight.Bold | |
| ), | |
| val body15: TextStyle = createTextStyle( |
🤖 Prompt for AI Agents
In core/src/main/java/com/nexters/knownknowns/core/theme/Type.kt around lines 51
to 56, the code defines a body16 TextStyle but does not define a body16Bold
variant, yet the example usage in MainActivity references body16Bold. To fix
this, either add explicit bold variants like body16Bold by creating a new
TextStyle with fontWeight set to Bold, or update the example usage to use
body16.copy(fontWeight = FontWeight.Bold) instead of body16Bold.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
개발자의 정석 준영오빠의 코드리뷰 바탕으로 수정 완료했습니다! 확인부탁드려요! @HamBP
따라서 디자인시스템 사용방식은 아래처럼 바뀌었습니다. Text(
text = "Hello $name!",
modifier = modifier,
style = KnownKnownsTheme.typography.head28.copy(fontWeight = FontWeight.Bold),
color = KnownKnownsTheme.colors.statePositivePrimary
) |
Issue
Work Description
To Reviewers
KnownKnownsTheme이라는 이름은 서비스명 확정되면 바꿔도 될 것 같아요!Summary by CodeRabbit
New Features
Refactor
Chores