로그인 화면 디자인 적용 #81
로그인 화면 디자인 적용 #81
Conversation
- 로그인 화면 UI를 구현했습니다. - 로고, 캐릭터 이미지, 타이틀 문구를 추가했습니다.
📝 WalkthroughWalkthrough이 변경은 로그인 화면 관련 리소스와 UI 컴포넌트의 추가·수정입니다. 두 개의 벡터 drawable(ic_app_logo.xml, ic_keepi_singing.xml)이 core/design-system에 추가되었고, LoginScreen 컴포저블이 Box 기반에서 Scaffold 기반으로 레이아웃을 변경하여 innerPadding을 적용하도록 리팩토링되었습니다. LoginButton에 외부 Modifier 파라미터가 추가되어 호출자에서 레이아웃 수정이 가능해졌으며, 로그인 타이틀 문자열 리소스(login_title_message)가 feature/login의 values에 추가되었습니다. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 `@feature/login/src/main/java/com/twix/login/LoginScreen.kt`:
- Around line 99-104: Image composable is passing modifier = Modifier which is
redundant and may leave the image unconstrained; either remove the modifier
parameter entirely from the Image(...) call (since Modifier is default) or
replace it with a size/constraint modifier (e.g., Modifier.size/width/height or
Modifier.fillMaxWidth with aspectRatio) to limit the intrinsic 390x420dp
drawable; locate the Image(...) that uses
ImageVector.vectorResource(...com.twix.designsystem.R.drawable.ic_keepi_singing)
and update the modifier accordingly.
- Around line 66-117: The Column content can overflow on small screens; update
the composable by making the parent Column scrollable (add
Modifier.verticalScroll(rememberScrollState())) or convert the large Spacers to
flexible space using Modifier.weight(1f) so elements (e.g., LoginButton) aren’t
pushed off-screen, and constrain the ic_keepi_singing Image by applying a
responsive modifier (e.g., sizeIn/maxHeight or fillMaxWidth with aspectRatio) so
its intrinsic 390x420dp won’t force overflow; locate these changes around the
Column, the Spacer usages, the Image referencing
com.twix.designsystem.R.drawable.ic_keepi_singing, and the LoginButton
invocation.
🧹 Nitpick comments (4)
core/design-system/src/main/res/drawable/ic_app_logo.xml (1)
1-29: Dark Mode 지원 관련 고려 사항로고의
fillColor가#171717로 하드코딩되어 있습니다. 현재 LoginScreen에서CommonColor.White배경을 명시적으로 설정하고 있어 당장은 문제가 되지 않지만, 향후 Dark Mode를 지원하게 되면 어두운 배경 위에서 로고가 보이지 않을 수 있습니다.필요한 경우 색상 리소스(
@color/...)나 테마 속성(?attr/...)을 활용하여 테마에 따라 색상이 변경되도록 하는 방법도 있습니다.As per coding guidelines, 디자인 시스템 리뷰 가이드: "Dark Mode 지원 여부"
core/design-system/src/main/res/drawable/ic_keepi_singing.xml (1)
1-242: 복잡한 벡터 드로어블의 성능 고려이 벡터 드로어블은
390x420dp크기에 약 40개 이상의 복잡한 베지에 곡선 path를 포함하고 있습니다. Android에서 복잡한 벡터 드로어블은 매 프레임마다Canvas에 그려지므로, 저사양 기기에서 렌더링 비용이 발생할 수 있습니다.로그인 화면이라 리스트처럼 반복 렌더링되는 것은 아니므로 당장 큰 문제는 아니지만, 만약 성능 이슈가 관찰된다면 WebP 비트맵 이미지로 대체하는 것을 고려해볼 수 있습니다. Android 공식 문서에서도 복잡한 일러스트레이션의 경우 비트맵을 권장합니다.
feature/login/src/main/java/com/twix/login/LoginScreen.kt (2)
78-84: 앱 로고에contentDescription추가를 권장합니다.
ic_app_logo는 앱의 브랜딩 요소로, 스크린 리더 사용자에게 앱 이름을 전달하는 것이 접근성 관점에서 좋습니다. 순수 장식 요소인ic_keepi_singing은null로 두는 것이 맞지만, 로고는 의미가 있는 이미지입니다.예시
Image( imageVector = ImageVector.vectorResource(com.twix.designsystem.R.drawable.ic_app_logo), - contentDescription = null, + contentDescription = stringResource(R.string.app_name), modifier = Modifier .padding(start = 24.dp), )As per coding guidelines, 디자인 시스템 리뷰 가이드: "접근성(Accessibility) 고려 여부"
106-114: 주석 처리된 코드 정리 제안
LoginType.entries.forEach반복문이 주석 처리되어 있고LoginButton.kt에도 KAKAO 관련 TODO가 있는 것으로 보아, 카카오 로그인은 추후 구현 예정인 것으로 보입니다. 주석 처리된 코드가 길게 남아 있으면 가독성을 해칠 수 있으니, 카카오 로그인 구현 시 다시 추가하는 것이 깔끔할 것 같습니다. 다만 곧 구현 예정이라면 현 상태도 괜찮습니다.
| Scaffold( | ||
| modifier = Modifier.fillMaxSize(), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| ) { innerPadding -> | ||
| Column( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 20.dp), | ||
| verticalArrangement = Arrangement.spacedBy(12.dp), | ||
| .padding(innerPadding) | ||
| .fillMaxSize() | ||
| .background(CommonColor.White), | ||
| ) { | ||
| Spacer(Modifier.height(35.dp)) | ||
|
|
||
| Image( | ||
| imageVector = ImageVector.vectorResource(com.twix.designsystem.R.drawable.ic_app_logo), | ||
| contentDescription = null, | ||
| modifier = | ||
| Modifier | ||
| .padding(start = 24.dp), | ||
| ) | ||
|
|
||
| Spacer(Modifier.height(24.dp)) | ||
|
|
||
| AppText( | ||
| text = stringResource(R.string.login_title_message), | ||
| style = AppTextStyle.H3, | ||
| color = GrayColor.C500, | ||
| modifier = | ||
| Modifier | ||
| .padding(start = 24.dp), | ||
| ) | ||
|
|
||
| Spacer(Modifier.height(27.dp)) | ||
|
|
||
| Image( | ||
| imageVector = ImageVector.vectorResource(com.twix.designsystem.R.drawable.ic_keepi_singing), | ||
| contentDescription = null, | ||
| modifier = | ||
| Modifier, | ||
| ) | ||
|
|
||
| // LoginType.entries.forEach { type -> | ||
| LoginButton(type = LoginType.GOOGLE, onClickLogin = onClickLogin) | ||
| LoginButton( | ||
| type = LoginType.GOOGLE, | ||
| onClickLogin = onClickLogin, | ||
| modifier = | ||
| Modifier | ||
| .padding(horizontal = 20.dp), | ||
| ) | ||
| // } | ||
|
|
||
| Spacer(Modifier.height(27.dp)) | ||
| } |
There was a problem hiding this comment.
다양한 화면 크기에서 레이아웃 오버플로우 가능성
현재 Column에 고정 높이의 Spacer들과 내재 크기 390x420dp인 ic_keepi_singing 이미지가 포함되어 있습니다. 작은 화면의 기기에서는 전체 콘텐츠가 화면을 초과하여 LoginButton이 잘리거나 보이지 않을 수 있습니다.
개선 방향으로 두 가지를 제안드립니다:
Column에verticalScroll을 추가하거나,Spacer에Modifier.weight(1f)를 활용하여 유연한 레이아웃을 구성- 이미지에 크기 제약을 추가하여 화면 비율에 맞게 조절
예시: weight를 활용한 유연한 레이아웃
) {
Spacer(Modifier.height(35.dp))
Image(
imageVector = ImageVector.vectorResource(com.twix.designsystem.R.drawable.ic_app_logo),
contentDescription = null,
modifier =
Modifier
.padding(start = 24.dp),
)
Spacer(Modifier.height(24.dp))
AppText(
text = stringResource(R.string.login_title_message),
style = AppTextStyle.H3,
color = GrayColor.C500,
modifier =
Modifier
.padding(start = 24.dp),
)
- Spacer(Modifier.height(27.dp))
+ Spacer(Modifier.weight(1f))
Image(
imageVector = ImageVector.vectorResource(com.twix.designsystem.R.drawable.ic_keepi_singing),
contentDescription = null,
- modifier =
- Modifier,
+ modifier = Modifier.fillMaxWidth(),
)
LoginButton(
type = LoginType.GOOGLE,
onClickLogin = onClickLogin,
modifier =
Modifier
.padding(horizontal = 20.dp),
)
Spacer(Modifier.height(27.dp))
}🤖 Prompt for AI Agents
In `@feature/login/src/main/java/com/twix/login/LoginScreen.kt` around lines 66 -
117, The Column content can overflow on small screens; update the composable by
making the parent Column scrollable (add
Modifier.verticalScroll(rememberScrollState())) or convert the large Spacers to
flexible space using Modifier.weight(1f) so elements (e.g., LoginButton) aren’t
pushed off-screen, and constrain the ic_keepi_singing Image by applying a
responsive modifier (e.g., sizeIn/maxHeight or fillMaxWidth with aspectRatio) so
its intrinsic 390x420dp won’t force overflow; locate these changes around the
Column, the Spacer usages, the Image referencing
com.twix.designsystem.R.drawable.ic_keepi_singing, and the LoginButton
invocation.
1 participant
이슈 번호
#80
리뷰/머지 희망 기한 (선택)
작업내용
결과물
리뷰어에게 추가로 요구하는 사항 (선택)