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

[Feat/#12] 공통 컴포넌트 button 구현 #13

Merged
merged 16 commits into from
Jan 13, 2025
Merged

Conversation

Roel4990
Copy link
Member

Related issue 🛠

Work Description ✏️

  • 공통 컴포넌트 button 구현

Screenshot 📸

image

To Reviewers 📢

Preview 때문에 코드가 좀 길 수 있습니다...

@Roel4990 Roel4990 added 🧡세홍🧡 🧡세홍🧡 FEAT✨ 새로운 기능 구현 labels Jan 11, 2025
@Roel4990 Roel4990 self-assigned this Jan 11, 2025
@Roel4990 Roel4990 requested a review from a team as a code owner January 11, 2025 15:18
@Roel4990 Roel4990 linked an issue Jan 11, 2025 that may be closed by this pull request
1 task
Copy link
Collaborator

@Hyobeen-Park Hyobeen-Park left a comment

Choose a reason for hiding this comment

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

버튼 컴포넌트 만들기!!! 정말 수고 많으셨습니다ㅎㅎ 코드 길이 보고 깜짝놀랐는데 프리뷰가 많아서 안심하고 봤어요😊

}
}
val paddingValues = when (size) {
ButtonSize.Xlarge -> PaddingValues(horizontal = 16.dp, vertical = 16.5.dp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1: 소수점 padding을 가지고 있네요!! 저희는 소수점은 최대한 지양해야 합니다!! 현재 GUI 상에서도 소수점으로 설정되어 있는데 이런 경우 디자인 쌤들께 수정 요청드리면 됩니다!!

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 부분은 요청드린 상태이며 완료되면 바로 수정할게요!

Box(
modifier = modifier
.fillMaxWidth()
.clip(RoundedCornerShape(8.dp))
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1: 버튼마다 radius 값이 달라서 피그마 다시 한번만 확인 부탁드릴게요!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다! 감사합니다.


Box(
modifier = modifier
.fillMaxWidth()
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2: fillMaxWidth() 나중에 컴포넌트를 사용할 때 받아오는 것이 더 유연한 컴포넌트가 될 것 같아요! fillMaxWidth()를 적용하지 않는 버튼이 있을 수도 있으니까요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다! 감사합니다!

else -> SpoonyAndroidTheme.colors.white
}

Box(
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: Button이 아닌 Box로 직접 구현하셨네요!! 혹시 특별한 이유가 있을까요?

저는 특별한 이유가 있는 것이 아니라면 컴포넌트를 만들 때 그에 맞는 컴포저블을 사용하는 편입니다(ex. 버튼 컴포넌트는 Button()으로!) 이 방식이 좀 더 컴포저블의 역할에 맞게 사용하는 것 같아서요ㅎㅎ 그리고 Button으로 만들면 위에 있는 로직들도 좀 더 가볍게 만들 수 있을 것 같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: Row를 Box로 한번 더 감싼 이유가 궁금해요! Row로도 충분히 구현 가능해보여서요..!!

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 그렇네요 Row로만 구현가능하네요. 수정했습니다! Row로만 구현하고 내부에 noRipple 넣으면 되겠네요..!

@@ -0,0 +1,19 @@
package com.spoony.spoony.core.util.modifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: 지금과 같은 패키지에서는 안에 들어갈 수 있는 파일이 ModifierExt 외에는 생각나는게 없어서,, 패키지를 나눈 의미가 사라질 것 같아요!!

  1. Modifier.noRippleClickable()은 Modifier의 확장함수이고 파일명도 ModifierExt이니 패키지를 extension이라고 네이밍하는건 어떤가요? 이런 경우 다른 확장함수(ex. ContextExt)들이 이 패키지 안에 들어가면 되겠네요!

  2. 혹은 util 내부에 바로 NoRippleClickable 이런 네이밍의 파일을 바로 넣는 경우도 자주 봤습니다ㅎㅎ 이런 경우 새로운 modifier 확장함수가 생길 때마다 새로운 파일이 생기겠네요!!

저는 주로 1번 방식으로 많이 개발했어요!! 다만 이거 정답은 아니니까ㅎㅎ 어떤게 더 좋을지 다른 분들의 의견도 궁금해요!!

Copy link
Member

@chattymin chattymin Jan 11, 2025

Choose a reason for hiding this comment

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

저도 두 방법 모두 좋다고 생각해요!
하지만 말씀해주신 대로 2번으로 할 경우 너무 많은 파일이 생성되어 저도 1번으로 주로 개발했었습니다 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 1번이 더 좋을 듯 싶네요! 수정하겠습니다!

@Composable
fun Modifier.noRippleClickable(
onClick: () -> Unit,
interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

P4: interactionSource를 받아오는 이유가 궁금합니다!! 이미 noRippleClickable은 interaction을 없애는 확장함수라고 생각해서 interactionSource를 받아올 필요가 없지 않나 하는 생각이 드네요!! 다른 분들 의견은 어떤가요?

Copy link
Member

Choose a reason for hiding this comment

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

저도 해당 값을 외부에서 받아오는 이유는 없다고 생각해요.

네이밍이 noRipple이기 때문에 추가적인 기능을 커스텀 하기보다는, 리플을 없애는 기능 하나만 하는게 더 좋다고 생각합니다 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하겠습니다!
버튼에 Pressed 색상 넣으려고 파라미터로 넣으면 좋을듯 싶어서 넣었는데, noRipple 이니 해당 조건에서는 무조건 Ripple 이 안되도록 하는게 맞는 것 같습니다.

import androidx.compose.ui.composed

@Composable
fun Modifier.noRippleClickable(
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: inline 함수로 바꿔주면 좋을 것 같아요!! 저도 아직 완벽하게 아는 것은 아니긴 하지만ㅎㅎ inline 함수는 람다식을 전달할 때 불필요한 객체를 생성하지 않아 성능상 더 좋다고 하네요!!

Copy link
Member

Choose a reason for hiding this comment

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

조금 설명을 덧붙이자면 inline함수를 사용하면 해당 함수를 호출하는 곳에서 내부 코드를 그대로 작성한 것 처럼 됩니다. 그렇기에 컴파일 코드 수는 길어지지만, 새 객체를 생성하지 않고 비용이 비싼 고차 함수를 런타임 패널티 없이 사용할 수 있게 해줘요

그래서 저도 inline을 사용하는 것을 추천 드립니다! 아마 inline을 붙이면 오류가 발생하실텐데 crossline이라는 키워드도 공부해시면 좋을것 같아요 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 참고해서 수정하겠습니다! 좋은 꿀팁 알아갑니다!

Copy link
Member

@chattymin chattymin left a comment

Choose a reason for hiding this comment

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

컴포넌트 만드느라 고생하셨습니다!
꼼꼼하게 Preview 만들어주신 덕분에 추후 유지보수가 편해질 것 같네요

수정하시고 다시 한번 요청 부탁드립니다 :)

Comment on lines 47 to 64
val backgroundColor = when {
!enabled -> when (style) {
ButtonStyle.Primary -> SpoonyAndroidTheme.colors.main100
ButtonStyle.Secondary -> SpoonyAndroidTheme.colors.gray300
ButtonStyle.Tertiary -> SpoonyAndroidTheme.colors.gray100
}
isPressed -> when (style) {
ButtonStyle.Primary -> SpoonyAndroidTheme.colors.main500
ButtonStyle.Secondary -> SpoonyAndroidTheme.colors.gray800
ButtonStyle.Tertiary -> SpoonyAndroidTheme.colors.gray100
}

else -> when (style) {
ButtonStyle.Primary -> SpoonyAndroidTheme.colors.main400
ButtonStyle.Secondary -> SpoonyAndroidTheme.colors.black
ButtonStyle.Tertiary -> SpoonyAndroidTheme.colors.gray0
}
}
Copy link
Member

@chattymin chattymin Jan 11, 2025

Choose a reason for hiding this comment

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

p1) when문의 값에 따라 특정 값을 결정하는 경우 remember로 묶어주어야 Recomposition을 방지해줄 수 있습니다. Icon 컴포넌트의 내부를 보시면 아래처럼 tint 색상에 따라 remember로 묶어주어 리컴포지션을 막아주고 있는 모습을 볼 수 있어요

// material3.Icon 내부 코드 일부 발췌
val colorFilter = remember(tint) {
    if (tint == Color.Unspecified) null else ColorFilter.tint(tint)
}

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 113 to 116
@Preview
@Composable
private fun SpoonyButtonPrimaryEnabledPreview() {
SpoonyAndroidTheme {
Copy link
Member

Choose a reason for hiding this comment

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

p5) 파라미터에 따라 Preview 종류가 늘어난다면 PreviewParameterProvider를 사용해보는건 어떨까요? Preview를 조금 더 가독성있고 수정하기 편하게 만들어줍니다.

물론 이걸 공부하고 적용하시는 것도 리소스이기 때문에 적용하지 않으셔도 좋아요!

제가 droidknights에 contribute했던 PR을 링크 걸어둘테니 적용하셔도 좋고 안하셔도 좋습니다 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

와 드나 컨트리뷰터,, 존경합니다.
최고 진짜 몰랐는데 꿀팁 알아갑니다. 이거 최곤데요

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

따봉 감사합니다!

@@ -0,0 +1,19 @@
package com.spoony.spoony.core.util.modifier
Copy link
Member

@chattymin chattymin Jan 11, 2025

Choose a reason for hiding this comment

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

저도 두 방법 모두 좋다고 생각해요!
하지만 말씀해주신 대로 2번으로 할 경우 너무 많은 파일이 생성되어 저도 1번으로 주로 개발했었습니다 :)

@Composable
fun Modifier.noRippleClickable(
onClick: () -> Unit,
interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }
Copy link
Member

Choose a reason for hiding this comment

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

저도 해당 값을 외부에서 받아오는 이유는 없다고 생각해요.

네이밍이 noRipple이기 때문에 추가적인 기능을 커스텀 하기보다는, 리플을 없애는 기능 하나만 하는게 더 좋다고 생각합니다 :)

import androidx.compose.ui.composed

@Composable
fun Modifier.noRippleClickable(
Copy link
Member

Choose a reason for hiding this comment

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

조금 설명을 덧붙이자면 inline함수를 사용하면 해당 함수를 호출하는 곳에서 내부 코드를 그대로 작성한 것 처럼 됩니다. 그렇기에 컴파일 코드 수는 길어지지만, 새 객체를 생성하지 않고 비용이 비싼 고차 함수를 런타임 패널티 없이 사용할 수 있게 해줘요

그래서 저도 inline을 사용하는 것을 추천 드립니다! 아마 inline을 붙이면 오류가 발생하실텐데 crossline이라는 키워드도 공부해시면 좋을것 같아요 :)

Comment on lines 35 to 44
@Composable
fun SpoonyButton(
text: String,
size: ButtonSize = ButtonSize.Medium,
style: ButtonStyle = ButtonStyle.Primary,
enabled: Boolean = true,
icon: (@Composable () -> Unit)? = null,
onClick: () -> Unit = {},
modifier: Modifier = Modifier
) {
Copy link
Member

Choose a reason for hiding this comment

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

p1) Button이라는 컴포넌트의 onClick이 옵셔널인것이 컴포넌트의 존재 이유와 맞지 않는 것 같습니다. 필수 파라미터로 하는게 버튼의 목적에 맞습니다

p1) modifier는 모든 옵셔널 파라미터 중 최상단으로 가야합니다. ref

Copy link
Member Author

Choose a reason for hiding this comment

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

disabled 사용할땐 안넣어도 된다고 생각해서 그렇게 했는데 크게 {} 라도 넣는게 좋을 듯 싶네요 감사합니다 수정했습니다!

Copy link
Member

Choose a reason for hiding this comment

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

disabled을 사용할때가 있나요?

onClick: () -> Unit = {},
modifier: Modifier = Modifier
) {
val interactionSource = remember { MutableInteractionSource() }
Copy link
Member

Choose a reason for hiding this comment

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

p3) interactionSource를 내부 선언하신 이유가 있을까요?
이 값은 매개변수에 넣어두는 것도 좋을 것 같습니다. 특정 버튼에는 다른 인터렉션을 넣어야 하는 등 추후 확장성을 챙길 수 있을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다! 확장성 고려한다면 해당 말씀대로 작업하는 것이 좋은 듯 싶습니다!

Copy link
Member

@chattymin chattymin left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!
이전에 제가 확인하지 못해서 놓쳤던 크리티컬한 이슈가 있어서 다시한번만 수정하고 요청 부탁드려요!

Comment on lines 95 to 99
.clickable(
indication = null,
interactionSource = interactionSource,
onClick = onClick
)
Copy link
Member

Choose a reason for hiding this comment

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

p1) enable에 대한 처리가 없는 것 같습니다. enable이 false여도 클릭하면 해당 액션이 실행되는 것 같은데 수정 부탁드려요!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하겠습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다!

}
}
}
val paddingValues = when (size) {
Copy link
Member

Choose a reason for hiding this comment

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

p2) color말고 밑에있는 padding이나 style이나 이런것들도 다 해달라는 의미였습니다. remember 처리 부탁드려요

그리고 paddingValue, ButtonSize, cornerRadius의 경우 같은 분기처리를 하고있으니 하나로 합쳐도 될 것같네요. 구조분해 적용하면 될것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

pressed 나 enabled 같은 경우에는 화면에서 바뀔 수 있다 생각해서 remember 을 통해 recomposable 을 사용하는게 맞다고 생각이 들었는데 나머지 padding 이나 style 같은 경우에는 바뀔일이 없으니 상관 없지 않을까 생각해서 안바꿨었습니다!
그래서 위에 보시면 remember 조건에 style은 안넣었어요!

Copy link
Member Author

Choose a reason for hiding this comment

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

제가 개념을 잘못 이해한 부분이 있을 수 있어서,, 여쭤보는 부분입니다!

Copy link
Member

Choose a reason for hiding this comment

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

remember를 통해 recomposable을 발생시키는 것이 아니라 recomposable을 막는 목적입니다.
해당 컴포넌트의 다른 속성이 변경되었을 때 remember되어있다면 다시 연산하지 않고 넘어가기 때문에 recomposable을 막을 수 있어서 사용하는거라 해당 값이 변경될 여지가 적어보여도 넣는게 좋습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다! 수정할게요!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정완료했습니다!

this.clickable(
indication = null,
interactionSource = remember { MutableInteractionSource() },
onClick = { onClick() }
Copy link
Member

Choose a reason for hiding this comment

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

p3) 이 부분 이렇게도 되지 않나요? 불필요한 람다를 추가로 만들필요는 없을 것 같습니다.

Suggested change
onClick = { onClick() }
onClick = onClick

Copy link
Member Author

Choose a reason for hiding this comment

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

람다 전달과 호출의 차이

람다 전달:

onClick = onClick

여기서 onClick은 람다를 전달만 하고 호출은 나중에 이뤄집니다.
clickable 내부에서 이 람다를 호출할 때까지 실행되지 않습니다.

람다 호출:

onClick = { onClick() }

전달받은 onClick 람다를 호출하도록 명시적으로 정의합니다.
{} 내부에서 실행 로직을 추가하거나 호출을 제어할 수 있습니다.

왜 inline과 crossinline을 사용했는가?

inline: 함수 호출 오버헤드를 줄이고, noRippleClickable 함수가 자주 호출될 때 성능을 최적화.
전달받은 onClick 람다에 대한 익명 객체 생성을 방지.
crossinline: onClick에서 비지역 반환이 발생할 가능성을 방지.
예를 들어, return을 사용해 noRippleClickable 바깥의 함수를 종료하려고 할 때 오류를 방지.

결론

전달받은 람다를 명시적으로 호출해야 하기 때문에 onClick 을 사용하지 않고 { onClick() } 로 해야된다고 합니다.

image

일단 잘몰라서 정리해봤는데.. 아직도 사실 이해가 잘 가진 않습니다.

Copy link
Member

@chattymin chattymin Jan 12, 2025

Choose a reason for hiding this comment

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

이부분은 저도 잘 모르던 부분인데 찾아보고 공유하도록 하겠습니다!
덕분에 새로운걸 알아가네요 :)

@Roel4990 Roel4990 requested a review from chattymin January 12, 2025 09:16
Copy link
Collaborator

@angryPodo angryPodo left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 참고해서 저도 좋은 코드 작성하겠습니다~

onClick = onClick
)
.padding(paddingValues),
verticalAlignment = Alignment.CenterVertically,
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4:

.semantics { 
    contentDescription = "..." 
}

이런 접근성 챙겨주는것도 좋아보여요!

Copy link
Member Author

Choose a reason for hiding this comment

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

오,, 처음 봤습니다! Row도 contentDescription 를 추가할 수 있었군요! 아직 어떤식으로 작성해야될지 모르겠어서 추후에 추가하도록 하겠습니다!

Copy link
Member

@chattymin chattymin left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link
Collaborator

@Hyobeen-Park Hyobeen-Park left a comment

Choose a reason for hiding this comment

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

빨간딱지_찐최종

}
val paddingValues = remember(size) {
when (size) {
ButtonSize.Xlarge -> PaddingValues(horizontal = 16.dp, vertical = 16.dp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1: 이거 디쟌 수정되었으니 피그마 확인 부탁드립니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다!


val textStyle = remember(size) {
when (size) {
ButtonSize.Xlarge -> spoonyTypography.body1b
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1: 폰트도!! 수정사항 피그마에 반영되었으니 확인 부탁드려요~

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다!

@Roel4990 Roel4990 requested a review from Hyobeen-Park January 13, 2025 04:09
Copy link
Collaborator

@Hyobeen-Park Hyobeen-Park left a comment

Choose a reason for hiding this comment

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

LGTM~~!!

@Roel4990 Roel4990 merged commit b6d98d4 into develop Jan 13, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FEAT✨ 새로운 기능 구현 🧡세홍🧡 🧡세홍🧡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 공통 컴포넌트 Button 구현
4 participants