[FIX/#46] 탑앱바 컴포넌트 수정 #214
Conversation
Hyobeen-Park
requested review from
chattymin,
Roel4990 and
angryPodo
and removed request for
a team
| Icon( | ||
| imageVector = ImageVector.vectorResource(R.drawable.ic_arrow_left_24), | ||
| contentDescription = null, | ||
| tint = SpoonyAndroidTheme.colors.gray700, | ||
| modifier = Modifier | ||
| .size(32.dp) | ||
| .noRippleClickable(onBackButtonClick) | ||
| .padding(start = 12.dp) | ||
| ) |
There was a problem hiding this comment.
P1)
피그마를 확인해 보니, 아이콘 크기는 24x24이지만 터치 영역은 32x32로 설정하는 것이 맞을까요?
현재 코드에서는 터치영역이 잘못 할당 되어있는것 같고, 아이콘 크기도 피그마보다 작아진 것 같습니다.
| Icon( | |
| imageVector = ImageVector.vectorResource(R.drawable.ic_arrow_left_24), | |
| contentDescription = null, | |
| tint = SpoonyAndroidTheme.colors.gray700, | |
| modifier = Modifier | |
| .size(32.dp) | |
| .noRippleClickable(onBackButtonClick) | |
| .padding(start = 12.dp) | |
| ) | |
| Box( | |
| contentAlignment = Alignment.Center, | |
| modifier = Modifier | |
| .padding(start = 12.dp) | |
| .size(32.dp) | |
| .clickable(onClick = onBackButtonClick) | |
| ) { | |
| Icon( | |
| imageVector = ImageVector.vectorResource(R.drawable.ic_arrow_left_24), | |
| contentDescription = null, | |
| tint = SpoonyAndroidTheme.colors.gray700 | |
| ) | |
| } |
위와 같이 Box로 감싸서 사용하는 것은 어떨까요?
사실 IconButton도 내부적으로 이런 구조를 사용하고 있습니다. 😃
한 번 확인해 주시면 감사하겠습니다!
There was a problem hiding this comment.
와 터치영역 놓칠뻔했네요 꼼꼼한 감사합니다😊😊 말씀하신대로 아이콘 크기가 24.dp, 터치 영역은 32.dp가 맞아요..!! 그리고 확인해보니 아이콘 크기는 피그마에 있던 그대로 24.dp로 적용되어 있습니다!! 아마 프리뷰라서 작아보였던 것이 아닐까 싶네요??
IconButton이 Icon을 Box로 감싼 구조인거 처음 알았어요ㅎㅎ 사실 개인적으로는 사용하지 않아도 구현이 가능한 컴포저블 함수들은 최대한 쓰지 않는 것을 좋아하는데 이 경우 제안해주신대로 Box로 감싸는 것이 가독성 측면에서 이점이 커서 이렇게 수정하도록 하겠습니다!!
There was a problem hiding this comment.
아 그리고 IconButton 내부 코드 보다가 발견했는데요 Material3 문서를 보시면 아이콘 버튼의 터치 영역은 최소 48.dp여야 해서 IconButton도 최소크기가 48.dp로 되어있더라구요ㅎㅎ
알게 된 점 1: 우리 서비스는 Material Design을 따르지 않고 있어요
알게 된 점 2: 그래서 IconButton을 그대로 쓰면 디자인과 맞지 않을 것 같네요ㅎㅎ
*TextButton도 최소크기 있네요 디자인과 정확하게 일치하게 쓰려면 이것도 잘 확인하고 써야겠어요
여러분 모두 이거 참고하기!!!
There was a problem hiding this comment.
맞아요 IconButton 은 내부 패딩을 고정시키고 선택 범위도 지들 맘대로 해놔서 저도 잘 안쓰긴합니다ㅋㅋ
구현 방식을 참고하면 좋을 것 같아서 말씀드렸습니다ㅎㅎ 이것도 아티클 보다가 내부구현 보게된건데 링크를 못찾아서ㅠ
| navigationIcon = { | ||
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.SpaceBetween, |
There was a problem hiding this comment.
p2) SpaceBetween의 역할이 여기서 무엇인가요?
내부의 Row가 weigth(1f)이고, action이 들어간다면 무조건 action은 맨 오른쪽으로 붙을 것 같습니다.
| if (showBackButton) { | ||
| IconButton( | ||
| onClick = onBackButtonClick | ||
| Box( | ||
| contentAlignment = Alignment.Center, | ||
| modifier = Modifier | ||
| .padding(start = 20.dp) | ||
| .size(32.dp) | ||
| .noRippleClickable(onBackButtonClick) | ||
| ) { | ||
| Icon( | ||
| imageVector = ImageVector.vectorResource(R.drawable.ic_arrow_left_24), | ||
| contentDescription = null, | ||
| tint = SpoonyAndroidTheme.colors.gray700, | ||
| modifier = Modifier | ||
| .size(32.dp) | ||
| tint = SpoonyAndroidTheme.colors.gray700 | ||
| ) |
There was a problem hiding this comment.
p2) showBackButton이 들어간다면 다른 Icon에 대한 확장성이 떨어지는 것 같습니다. navigationIcon과 같이 확장성있게 만들어 두고, 해당하는 컴포넌트를 넣어주는건 어떨까요?
현재 상태라면 showBackButton은 넣고 onBackButtonClick을 빼먹는다거나, 그 반대의 상황이 발생할 수도 있을 것 같습니다.
| @Composable | ||
| fun SpoonyBasicTopAppBar( | ||
| modifier: Modifier = Modifier, | ||
| showBackButton: Boolean = false, | ||
| backgroundColor: Color = SpoonyAndroidTheme.colors.white, | ||
| onBackButtonClick: () -> Unit = {}, | ||
| actions: @Composable RowScope.() -> Unit = {}, | ||
| actions: @Composable () -> Unit = {}, |
There was a problem hiding this comment.
p4) RowScop를 뺀 이유가 있나요?
Related issue 🛠
Work Description ✏️
Screenshot 📸
UI는 동일합니다!
Uncompleted Tasks 😅
N/A
To Reviewers 📢