-
Notifications
You must be signed in to change notification settings - Fork 0
[UI/#15] 다이얼로그 및 Alert 버튼 컴포넌트 제작 #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthrough새로운 SmashingDialog Composable 컴포넌트가 추가되었습니다. 이 컴포넌트는 제목, 선택적 부제목, 동작 버튼을 포함하는 다이얼로그를 렌더링하며, 어두운 배경의 둥근 레이아웃을 사용합니다. 세 가지 미리보기가 포함되어 있습니다. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/src/main/java/com/smashing/app/core/designsystem/component/dialog/SmashingDialog.kt (4)
23-23: 사용되지 않는 import를 제거하세요.
noRippleClickable이 현재 파일에서 사용되지 않습니다.🔎 제안된 수정
-import com.smashing.app.core.extension.noRippleClickable
25-25: 디자인 시스템 등록 추적을 고려하세요.TODO 코멘트가 디자인 시스템 등록 후 변경 예정임을 나타냅니다. 이 작업이 추적되고 있는지 확인하세요.
이 TODO를 추적하기 위한 별도의 이슈를 생성하시겠습니까?
56-60: 하드코딩된 색상과 간격 값을 테마로 이동하는 것을 권장합니다.현재 배경색(
Color(0xFF252A36)), 텍스트 색상(Color.White), 둥근 모서리 반경(12.dp), 패딩 값들이 하드코딩되어 있습니다. 디자인 시스템 등록 시 이들을MaterialTheme의 색상 및 간격 값으로 대체하면 유지보수성과 테마 일관성이 향상됩니다.25번 줄의 TODO 코멘트와 관련된 작업으로 보입니다.
🔎 제안된 개선 방향
디자인 시스템 등록 후:
.background( color = MaterialTheme.colorScheme.surface, // 또는 커스텀 색상 shape = RoundedCornerShape(12.dp), // 또는 MaterialTheme.shapes.medium )Text( text = title, color = MaterialTheme.colorScheme.onSurface, style = MaterialTheme.typography.bodyLarge, )Also applies to: 64-64, 70-70
62-77: 제목과 부제목 사이의 간격을 추가하는 것을 고려하세요.현재 부제목이 있을 때 제목과 부제목 사이에
Spacer가 없어 두 텍스트가 너무 가깝게 보일 수 있습니다. 가독성 향상을 위해 제목과 부제목 사이에 작은 간격을 추가하는 것을 권장합니다.🔎 제안된 수정
Text( text = title, color = Color.White, style = MaterialTheme.typography.bodyLarge, ) if (subtitle != null) { + Spacer(modifier = Modifier.height(8.dp)) Text( text = subtitle, color = Color.White, style = MaterialTheme.typography.bodyMedium, textAlign = TextAlign.Center, ) Spacer(modifier = Modifier.height(20.dp)) } else { Spacer(modifier = Modifier.height(24.dp)) }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/smashing/app/core/designsystem/component/dialog/SmashingDialog.kt
⏰ 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). (1)
- GitHub Check: PR Checker
| SmashingAlertButton( | ||
| text = "text", | ||
| onClick = {}, | ||
| isPrimary = false, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's search for SmashingAlertButton definition in the codebase
rg "fun SmashingAlertButton" --type kotlinRepository: TEAM-SMASHING/SMASHING-ANDROID
Length of output: 56
🏁 Script executed:
# Also search more broadly for SmashingAlertButton
rg "SmashingAlertButton" --type kotlin -lRepository: TEAM-SMASHING/SMASHING-ANDROID
Length of output: 161
🏁 Script executed:
# Let's check the imports and content of the file in question
fd "SmashingDialog.kt" --type fRepository: TEAM-SMASHING/SMASHING-ANDROID
Length of output: 161
🏁 Script executed:
# Read the SmashingDialog.kt file to see imports and the code in question
cat -n "app/src/main/java/com/smashing/app/core/designsystem/component/dialog/SmashingDialog.kt" | head -150Repository: TEAM-SMASHING/SMASHING-ANDROID
Length of output: 5239
🏁 Script executed:
# Search for SmashingAlertButton import or class definition anywhere
rg "SmashingAlertButton" --type kotlin -B 2 -A 2Repository: TEAM-SMASHING/SMASHING-ANDROID
Length of output: 2451
SmashingAlertButton 컴포넌트가 정의되지 않았습니다.
이 컴포넌트는 Preview 함수들(lines 94, 111, 117, 135)에서 참조되고 있지만 코드베이스에 정의되지 않았으며 import되지 않았습니다. 현재 상태로는 컴파일 오류가 발생합니다. SmashingAlertButton을 정의하거나, 올바른 라이브러리에서 import해야 합니다.
seungjunGong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋네요 우선 어푸 남겨 놓겠습니다 굿굿굿!!
| Row( | ||
| horizontalArrangement = Arrangement.spacedBy(10.dp, Alignment.CenterHorizontally), | ||
| modifier = Modifier.fillMaxWidth(), | ||
| content = buttonContent, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2: 의견 드립니다!
현재 buttonContent: @Composable RowScope.() -> Unit 방식은 매번 버튼 컴포저블을 구현해서 넣어줘야하기에 번거로움이 추가될 수 있다고 생각하는데요
대안으로, 디자인 상 Dialog가 크게 2가지 타입(1버튼: “확인”, 2버튼: “아니요 | 예”) 으로 보이는데요. 이 경우 enum class로 타입을 명시해서 컴포넌트 내부에서 버튼 구성/간격/정렬을 고정하는 방식도 괜찮아 보입니다.
예) enum class DialogType { AIERT, CONFIRM }
→ CONFIRM이면 확인 버튼 1개, AIERT이면 아니요/예 2개 버튼을 보여줌
이렇게 하면 사용처에서는 타입만 선택하면 되고, 버튼 레이아웃 규칙이 한 곳에 모여 일관성 유지 + 사용 실수 방지 측면에서 더 깔끔할 것 같습니다.
정답은 없기에 지금 방식이 더 나아보이면 해당 방식으로 진행하는 것도 괜찮습니다 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호... 함 수정해볼게요!
vahkjsdf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했습니당 짱짱
ShinHyeongcheol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!!
RowScope를 사용한 방식이 인상깊었어요~
seungjunGong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 추가로 남겼습니다 ~!!
| * @param isPrimary true일 경우 파란색(Primary), false일 경우 회색(Secondary) 배경이 적용 | ||
| */ | ||
| @Composable | ||
| fun SmashingAlertButton( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2: 제가 놓쳤었는데 .. 피그마 상에 지금 보니까 해당 버튼을 사용하는곳이 다이얼로그 뿐인거 같아서 지금 상태에서는 SmashingDialog 파일 안 아래에 두고 사용하는건 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!!
| color = if (isPrimary) Color(SmashingTheme.colors.btnBgSecondaryActive.value) else Color( | ||
| SmashingTheme.colors.btnBgTertiaryActive.value | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p1: 여기는 상위 컴포저블에서 색상을 파라미터로 전달해주는게 나중에 확장성있게 구현하는데 더 좋을 것 같습니다 !!
추가로 color 지정할때는 Color() 로 묶을 필요없이 바로 SmashingTheme.colors.btnBgSecondaryActive 넣어주시면 돼요!
| color = if (isPrimary) Color(SmashingTheme.colors.btnBgSecondaryActive.value) else Color( | ||
| SmashingTheme.colors.btnBgTertiaryActive.value | ||
| ), | ||
| shape = RoundedCornerShape(8.dp), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p1: round 값이 10.dp 인거 같은데 확인 부탁드립니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
허거딩스 ㅠ 그렇네요 수정하겠습니다
| color = SmashingTheme.colors.txtPrimary, | ||
| style = SmashingTheme.typography.md.semibold16, | ||
| ) | ||
| if (subtitle != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3: 가독성을 위해 컴포저블함수 사이 줄바꿈 넣으면 좋을것 같아요
| when (type) { | ||
| DialogType.CONFIRM -> { | ||
| SmashingAlertButton( | ||
| text = confirmText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p3: text 가 다이얼로그에서 고정 값이 아니요|예 와 같이 고정되어 있는 값이 있는거 같은데 여기 따로 구분하신 이유가 있으신가요? 단순 궁금증입니다 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
N/A
To Reviewers 📢
감사합니당
✏️ Tip: You can customize this high-level summary in your review settings.