-
Notifications
You must be signed in to change notification settings - Fork 2
[Feat/#12] 공통 component 생성 #17
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
sonms
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.
고생하셨습니다!!!
| modifier: Modifier = Modifier, | ||
| ) { | ||
| Box( | ||
| Modifier |
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.
올바른 체이닝을 위해 상위에서 받은 modifier를 가장 먼저 적용해주세요
|
|
||
| @Preview | ||
| @Composable | ||
| fun PreviewSubChip() { |
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.
preview는 미리보기용 함수기 때문에 의도치 않은 사용 방지, 캡슐화를 위해 private로 바꿔주세요
| color = Color.Gray, | ||
| shape = RoundedCornerShape(4.dp) | ||
| ) | ||
| .padding(6.dp, 4.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.
큰 문제는 없지만 padding에 named arguments 사용을 권장합니다잉 verical = / horizontal =
| disabledContentColor = Color.DarkGray | ||
| ) | ||
| ) { | ||
| Text(text = 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.
여기도 색상 적용 가능하게 변경부탁드려용
| text: String, | ||
| enabled: Boolean, | ||
| modifier: Modifier = Modifier, | ||
| onClick: () -> Unit, |
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.
Compose API guideline에 따르면 인자의 순서는
필수 인자
Modifier
선택인자
(그 중에서도 @composable은 마지막)
를 권장하고 있으니 전체적으로 한 번 적용해보는 것은 어떨까요?
| modifier: Modifier = Modifier, | ||
| onClick: () -> Unit, | ||
| ) { | ||
| Button( |
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.
저는 Button을 사용하기보다는 Text로 사용하는 편입니다! 이렇게 사용하면 버튼의 절대적인 높이를 활용하기 보다 padding이나 text를 활용해서 스크린에 따라 반응되니까요!
| .clip(RoundedCornerShape(8.dp)), | ||
| 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.
modifier = Modifier
.fillMaxWidth(),
shape = RoundedCornerShape(8.dp),
이런 식으로 clip을 제거해도 원하는 결과를 얻을 수 있었을 것 같습니다!
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| fun PreviewMainButtonV2() { |
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.
마찬가지로 private! 및 함수명 통일 부탁드려요 Preview + Composable 함수이름 등으로
name: pull_request_template
about: pr 생성용 템플릿입니다~
title: ''
labels: ''
assignees: ''
ISSUE
❗ WORK DESCRIPTIONAdd commentMore actions
📸 SCREENSHOT
📢 TO REVIEWERS