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

Top Bar 구현 #19

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Top Bar 구현 #19

wants to merge 19 commits into from

Conversation

HI-JIN2
Copy link
Member

@HI-JIN2 HI-JIN2 commented Dec 9, 2024

구현

image

ToReviewer

왼쪽 아이콘은 box로 감싸서 터치영역을 넓혔는데, actions으로 받은 오른쪽 친구들의 터치영역을 넓힐 수 있는 방법이 있을까요? 단순하게 요소 간의 간격 띄우는거는 horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), 로 하였는데, 다시 보니 여기도 왼쪽 아이콘 처럼 터치영역을 넓혀야할 것 같드라구요.. 좋은 아이디어가 있다면 코멘트 부탁드립니다. 감사합니다.

@HI-JIN2 HI-JIN2 changed the title ㅅop bar Top Bar 구현 Dec 9, 2024
@HI-JIN2 HI-JIN2 self-assigned this Dec 9, 2024
@HI-JIN2 HI-JIN2 marked this pull request as draft December 13, 2024 07:29
Copy link
Member

@kangyuri1114 kangyuri1114 left a comment

Choose a reason for hiding this comment

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

아직 진행중인것 같지만 리뷰도 한번 확인부탁드려요!

*
* @param title headline or Logo 최대 9자(공백 포함)
* @param navIcon 왼쪽 아이콘 NavIcon.None, NavIcon.Menu, NavIcon.Back
* @param actions 오른쪽 아이콘 or 텍스트 (아이콘은 임의로 변경할 수 있으며 Center-aligned의 우측엔 최대 2개의 아이콘 버튼)
Copy link
Member

Choose a reason for hiding this comment

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

아이콘 개수는 굳이 코드상에서 제한할 필요는 없을 것 같습니다..!
피그마 문서자체가 디자인 가이드이기도 해서요 디자인측에서 고려해서 넘겨주실 것 같아요
게일 PR에서는 require로 강제해주시긴 했는데, 의문이 있어 리뷰남겨둔 상황이라 한번 참고해주세요

Copy link
Member

Choose a reason for hiding this comment

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

이부분은 우선 강제하지 않는 걸로 가도 될 것 같습니다!

Spacer(modifier = Modifier.weight(1f))

// Actions
if (actions != null) {
Copy link
Member

Choose a reason for hiding this comment

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

별건 아니지만 전 이런식의 표현이 좀 더 kotlin스러워서 선호합니다

actions?.let{
    ... // anctions가 null이 아닌경우
}

Comment on lines +124 to +125
title: String,
actions: @Composable (RowScope.() -> Unit)? = null
Copy link
Member

Choose a reason for hiding this comment

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

여기서 modifier를 안넣은 이유가 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

놓친 것 같습니다! 수정했습니닷 🧐

Comment on lines 150 to 158
if (actions != null) {
Row(
verticalAlignment = Alignment.CenterVertically,
modifier = Modifier
.padding(10.dp)
) {
actions()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

오른쪽 아이콘이 여러개 들어간 경우 figma상 각각 아이콘 사이의 패딩과 현재 프리뷰 상 패딩이 다른 것 같아요
이것도 사용 시에 패딩을 주는 것 말고 패딩이 지정되어있으면 어떨까요
[프리뷰]
image

[피그마]
image
image

* 현재 페이지의 제목을 나타낼 때 사용합니다. 아이콘으로 기능을 명확하게 표현하지 못하거나 확실하게 기능을 설명하고 싶을 때 아이콘 대신 Text를 사용할 수 있습니다.
*
* @param title headline or Logo 최대 9자(공백 포함)
* @param actions 오른쪽 아이콘 or 텍스트 (아이콘은 임의로 변경할 수 있으며 Left-aligned의 우측엔 최대 3개의 아이콘 버튼)
Copy link
Member

Choose a reason for hiding this comment

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

actions보다는 다른 명칭이 어떨까용
왜 actions인가요??!

Copy link
Member Author

Choose a reason for hiding this comment

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

오른쪽에 들어가는 친구들은 주로 공유하기나 더보기, 신고하기 등 사용자가 주도적으로 액션을 취한다고 생각해서 action이라고 하였습니다! 사실 머테리얼 보고 그렇게 한 걸로 기억하는데, 혹시 추천할만한 네이밍이 있을까요?

Copy link
Member

Choose a reason for hiding this comment

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

흠... 그런 의미가 있다면 우선은 굳이 수정할 필요는 없을 것 같아요
사용자 관점에서는 actions가 맞는 것 같긴하지만 해당 컴포넌트를 사용하는 개발자 관점에서의 actions라고 했을때 마땅히 떠오르는 게 없었어서 여쭤봤어요!

…p-bar

# Conflicts:
#	compose/src/main/kotlin/com/yourssu/handy/compose/TopAppBar.kt
@HI-JIN2 HI-JIN2 marked this pull request as ready for review January 13, 2025 10:38
@cometj03
Copy link
Member

질문하신 내용에 대해서 머티리얼은 어떻게 구현되어 있나요?

Copy link
Member

@kangyuri1114 kangyuri1114 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!

*
* @param title headline or Logo 최대 9자(공백 포함)
* @param navIcon 왼쪽 아이콘 NavIcon.None, NavIcon.Menu, NavIcon.Back
* @param actions 오른쪽 아이콘 or 텍스트 (아이콘은 임의로 변경할 수 있으며 Center-aligned의 우측엔 최대 2개의 아이콘 버튼)
Copy link
Member

Choose a reason for hiding this comment

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

이부분은 우선 강제하지 않는 걸로 가도 될 것 같습니다!

actions?.let {
Row(
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), // 요소 사이에 24dp 간격 추가
Copy link
Member

Choose a reason for hiding this comment

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

👍

* 현재 페이지의 제목을 나타낼 때 사용합니다. 아이콘으로 기능을 명확하게 표현하지 못하거나 확실하게 기능을 설명하고 싶을 때 아이콘 대신 Text를 사용할 수 있습니다.
*
* @param title headline or Logo 최대 9자(공백 포함)
* @param actions 오른쪽 아이콘 or 텍스트 (아이콘은 임의로 변경할 수 있으며 Left-aligned의 우측엔 최대 3개의 아이콘 버튼)
Copy link
Member

Choose a reason for hiding this comment

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

흠... 그런 의미가 있다면 우선은 굳이 수정할 필요는 없을 것 같아요
사용자 관점에서는 actions가 맞는 것 같긴하지만 해당 컴포넌트를 사용하는 개발자 관점에서의 actions라고 했을때 마땅히 떠오르는 게 없었어서 여쭤봤어요!

@kangyuri1114
Copy link
Member

왼쪽 아이콘은 box로 감싸서 터치영역을 넓혔는데, actions으로 받은 오른쪽 친구들의 터치영역을 넓힐 수 있는 방법이 있을까요? 단순하게 요소 간의 간격 띄우는거는 horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), 로 하였는데, 다시 보니 여기도 왼쪽 아이콘 처럼 터치영역을 넓혀야할 것 같드라구요.. 좋은 아이디어가 있다면 코멘트 부탁드립니다. 감사합니다.

-> 내부 패딩을 넣는 방법은 어떨까요??

@HI-JIN2
Copy link
Member Author

HI-JIN2 commented Jan 14, 2025

왼쪽 아이콘은 box로 감싸서 터치영역을 넓혔는데, actions으로 받은 오른쪽 친구들의 터치영역을 넓힐 수 있는 방법이 있을까요? 단순하게 요소 간의 간격 띄우는거는 horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), 로 하였는데, 다시 보니 여기도 왼쪽 아이콘 처럼 터치영역을 넓혀야할 것 같드라구요.. 좋은 아이디어가 있다면 코멘트 부탁드립니다. 감사합니다.

-> 내부 패딩을 넣는 방법은 어떨까요??

https://www.notion.so/yourssu/top-bar-6840c966d0a24e47a3120ff7ebfba193?pvs=4
요걸 봐주시면 좋을 것 같은데..

  actions?.let {
            Row(
                verticalAlignment = Alignment.CenterVertically,
                horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), // 요소 사이에 24dp 간격 추가
                modifier = Modifier.fillMaxHeight()
            ) {
                Spacer(modifier = Modifier.weight(1f))
                actions()
            }
        }
        

actions: @Composable (RowScope.() -> Unit)? = null 이렇게 받아버리면 action의 각 요소에 접근을 못해서 내부 패딩을 줄 수가 없어요. (제가 모르는걸수도 있지만)
그래서 머테리얼이나 스카이스캐너 구현을 봤는데, IconAction, TextAction 이런식으로 별도의 이너 함수를 만들어서 그 형식대로 받더라구요. 이렇게 하면 내부 패딩을 줄 수는 있긴 한데, 호출시 IconAction, TextAction의 존재를 알아야한다는게 저한테는 별로 좋지 않게 느껴져서 ㅠㅠ

@kangyuri1114
Copy link
Member

kangyuri1114 commented Jan 14, 2025

actions: @composable (RowScope.() -> Unit)? = null 이렇게 받아버리면 action의 각 요소에 접근을 못해서 내부 패딩을 줄 수가 없다 -> 그런 것 같네요

다른 디자인시스템처럼 IconAction, TextAction 이런식으로 별도의 이너 함수를 만들어서 그 형식대로 받거나 왼쪽 아이콘처럼 Box로 감싸거나 해야할것같아요
Box로 감싸면 이런식으로..?

actions?.let {
    Row(
        verticalAlignment = Alignment.CenterVertically,
        horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), // 요소 간 간격
        modifier = Modifier.fillMaxHeight()
    ) {
        Spacer(modifier = Modifier.weight(1f))
        it.forEach { action ->
            Box(
                contentAlignment = Alignment.Center,
                modifier = Modifier
                    .size(48.dp) // 터치 영역 48dp 보장
                    .clickable { action.onClick() } // 각각의 action 클릭 핸들러
            ) {
                action.icon()
            }
        }
    }
}

@HI-JIN2
Copy link
Member Author

HI-JIN2 commented Jan 14, 2025

actions: @composable (RowScope.() -> Unit)? = null 이렇게 받아버리면 action의 각 요소에 접근을 못해서 내부 패딩을 줄 수가 없다 -> 그런 것 같네요

다른 디자인시스템처럼 IconAction, TextAction 이런식으로 별도의 이너 함수를 만들어서 그 형식대로 받거나 왼쪽 아이콘처럼 Box로 감싸거나 해야할것같아요 Box로 감싸면 이런식으로..?

actions?.let {
    Row(
        verticalAlignment = Alignment.CenterVertically,
        horizontalArrangement = Arrangement.spacedBy(topBarActionsInsidePadding), // 요소 간 간격
        modifier = Modifier.fillMaxHeight()
    ) {
        Spacer(modifier = Modifier.weight(1f))
        it.forEach { action ->
            Box(
                contentAlignment = Alignment.Center,
                modifier = Modifier
                    .size(48.dp) // 터치 영역 48dp 보장
                    .clickable { action.onClick() } // 각각의 action 클릭 핸들러
            ) {
                action.icon()
            }
        }
    }
}

it.forEach가 안먹..혀서 ㅜㅜ @Composable (RowScope.() -> Unit)? = null 이렇게 통짜로 받는거 자체가 그 자체로 실행 가능한 컴포저블 코드 블록이라서 안 먹히는 것 같아요. 이너함수 만드는 방법은 피하고 싶었는데... 이렇게 되면 왼쪽 아이콘 버튼도 동일하게 받도록 수정하는게 좋을 것 같군여..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants