Skip to content
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

[FIX/#46] 탑앱바 컴포넌트 수정 #214

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.spoony.spoony.core.designsystem.component.topappbar

import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.material3.Icon
@@ -45,7 +44,6 @@ fun CloseTopAppBar(
color = SpoonyAndroidTheme.colors.black,
modifier = Modifier
.padding(start = 20.dp)
.fillMaxWidth()
)
}
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package com.spoony.spoony.core.designsystem.component.topappbar

import androidx.compose.foundation.layout.RowScope
import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.material3.CenterAlignedTopAppBar
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.foundation.layout.statusBarsPadding
import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton
import androidx.compose.material3.TopAppBarDefaults
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.vector.ImageVector
@@ -16,40 +18,46 @@ import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import com.spoony.spoony.R
import com.spoony.spoony.core.designsystem.theme.SpoonyAndroidTheme
import com.spoony.spoony.core.util.extension.noRippleClickable

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun SpoonyBasicTopAppBar(
modifier: Modifier = Modifier,
showBackButton: Boolean = false,
backgroundColor: Color = SpoonyAndroidTheme.colors.white,
onBackButtonClick: () -> Unit = {},
actions: @Composable RowScope.() -> Unit = {},
actions: @Composable () -> Unit = {},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p4) RowScop를 뺀 이유가 있나요?

content: @Composable () -> Unit = {}
) {
CenterAlignedTopAppBar(
title = content,
modifier = modifier,
navigationIcon = {
Row(
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.SpaceBetween,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p2) SpaceBetween의 역할이 여기서 무엇인가요?
내부의 Row가 weigth(1f)이고, action이 들어간다면 무조건 action은 맨 오른쪽으로 붙을 것 같습니다.

modifier = modifier
.background(backgroundColor)
.fillMaxWidth()
.padding(vertical = 12.dp)
.statusBarsPadding()
) {
Row(
verticalAlignment = Alignment.CenterVertically,
modifier = Modifier
.weight(1f)
) {
if (showBackButton) {
IconButton(
onClick = onBackButtonClick
) {
Icon(
imageVector = ImageVector.vectorResource(R.drawable.ic_arrow_left_24),
contentDescription = null,
tint = SpoonyAndroidTheme.colors.gray700,
modifier = Modifier
.size(32.dp)
)
}
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)
)
Copy link
Collaborator

@angryPodo angryPodo Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1)
피그마를 확인해 보니, 아이콘 크기는 24x24이지만 터치 영역은 32x32로 설정하는 것이 맞을까요?

현재 코드에서는 터치영역이 잘못 할당 되어있는것 같고, 아이콘 크기도 피그마보다 작아진 것 같습니다.
image

Suggested change
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도 내부적으로 이런 구조를 사용하고 있습니다. 😃

image
한 번 확인해 주시면 감사하겠습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 터치영역 놓칠뻔했네요 꼼꼼한 감사합니다😊😊 말씀하신대로 아이콘 크기가 24.dp, 터치 영역은 32.dp가 맞아요..!! 그리고 확인해보니 아이콘 크기는 피그마에 있던 그대로 24.dp로 적용되어 있습니다!! 아마 프리뷰라서 작아보였던 것이 아닐까 싶네요??

IconButton이 Icon을 Box로 감싼 구조인거 처음 알았어요ㅎㅎ 사실 개인적으로는 사용하지 않아도 구현이 가능한 컴포저블 함수들은 최대한 쓰지 않는 것을 좋아하는데 이 경우 제안해주신대로 Box로 감싸는 것이 가독성 측면에서 이점이 커서 이렇게 수정하도록 하겠습니다!!

Copy link
Collaborator Author

@Hyobeen-Park Hyobeen-Park Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그리고 IconButton 내부 코드 보다가 발견했는데요 Material3 문서를 보시면 아이콘 버튼의 터치 영역은 최소 48.dp여야 해서 IconButton도 최소크기가 48.dp로 되어있더라구요ㅎㅎ

알게 된 점 1: 우리 서비스는 Material Design을 따르지 않고 있어요
알게 된 점 2: 그래서 IconButton을 그대로 쓰면 디자인과 맞지 않을 것 같네요ㅎㅎ

*TextButton도 최소크기 있네요 디자인과 정확하게 일치하게 쓰려면 이것도 잘 확인하고 써야겠어요

여러분 모두 이거 참고하기!!!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞아요 IconButton 은 내부 패딩을 고정시키고 선택 범위도 지들 맘대로 해놔서 저도 잘 안쓰긴합니다ㅋㅋ
구현 방식을 참고하면 좋을 것 같아서 말씀드렸습니다ㅎㅎ 이것도 아티클 보다가 내부구현 보게된건데 링크를 못찾아서ㅠ

}
},
actions = actions,
colors = TopAppBarDefaults.centerAlignedTopAppBarColors(
containerColor = backgroundColor
)
)
content()
}
actions()
}
}

@Preview
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.spoony.spoony.core.designsystem.component.topappbar

import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
@@ -28,7 +27,8 @@ fun TagTopAppBar(
LogoTag(
count = count,
tagSize = tagSize,
modifier = Modifier.padding(end = 20.dp)
modifier = Modifier
.padding(end = 20.dp)
)
},
content = content,
@@ -47,7 +47,7 @@ private fun TagTopAppBarPreview() {
Text(
text = "홍대입구역",
modifier = Modifier
.fillMaxWidth()
.padding(start = 20.dp)
)
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package com.spoony.spoony.core.designsystem.component.topappbar

import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import com.spoony.spoony.core.designsystem.theme.SpoonyAndroidTheme

@Composable
@@ -21,7 +25,11 @@ fun TitleTopAppBar(
Text(
text = title,
style = SpoonyAndroidTheme.typography.title2b,
color = SpoonyAndroidTheme.colors.black
color = SpoonyAndroidTheme.colors.black,
modifier = Modifier
.fillMaxWidth()
.padding(end = 44.dp),
textAlign = TextAlign.Center
)
},
modifier = modifier,
Original file line number Diff line number Diff line change
@@ -2,13 +2,12 @@ package com.spoony.spoony.presentation.explore.component

import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.material3.Icon
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.vector.ImageVector
import androidx.compose.ui.res.vectorResource
@@ -29,10 +28,10 @@ fun ExploreTopAppBar(
) {
Row(
horizontalArrangement = Arrangement.Start,
verticalAlignment = Alignment.CenterVertically,
modifier = Modifier
.padding(start = 20.dp)
.noRippleClickable(onClick = onClick)
.padding(vertical = 4.dp)
) {
Text(
text = "서울특별시 $place",
@@ -48,6 +47,5 @@ fun ExploreTopAppBar(
.padding(start = 5.dp)
)
}
Spacer(modifier = Modifier.fillMaxWidth())
}
}