-
Notifications
You must be signed in to change notification settings - Fork 0
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/#87] IconChip 구조 변경 #88
Conversation
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.
수고하셨습니다!! 그라데이션 때문에 생각보다 칩 로직이 복잡해졌네요... 코리 확인하시고 수정 부탁드릴게요!!
selectedTextColor: Color, | ||
unSelectedTextColor: Color, |
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.
P3: 텍스트 컬러는 칩들 모두 동일한 것 같아서 내부에서 처리해도 되지 않을까 싶어요..!!
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.
P1: 그리고 background 컬러를 안받아오고 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.
구조를 변경했어요! 확인 부탁드려요!
val iconUrl = if (isSelected) selectedIconUrl else unSelectedIconUrl | ||
|
||
val textColor = if (isSelected) selectedTextColor else unSelectedTextColor |
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.
P2: 이 부분을 value에 한번 더 저장하신 이유가 따로 있으신가요? 없다면 그냥 저장하지 않고 바로 넣어주는 것이 좋을 것 같습니다!!
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.
넵 수정할게요!
.then( | ||
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.
P1: 아 진짜 제가 알려줘놓고 이래서 정말 죄송한데요... 이렇게 하면 padding을 못넣네요... 이러면 안돼!!!!!!!!!!!!! 수정합시다ㅜㅜ
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.
수정했습니다!
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.
고생하셨습니다. 논의 후에 수정사항 반영해주세요~
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를 내부에 넣게 되면 패딩이나 외부에서 수정자를 사용할 수 없는 이슈가 있을 것 같아요.
서버에서 오는 값을 보면
"categoryName": "로컬 수저",
"iconUrlNotSelected": "url_black_2",
"iconUrlSelected": "url_white_2"
이런 형식인데
IconChip(
text:String,
selectedIconUrl: String,
unSelectedIconUrl: String,
isGradient: Boolean = false
) {
// isGradient = true일경우 그라디언트 배경 기본설정
// else의 경우 backGroundColor = Solid Color
}
이 구조로 가는게 어떨까요? 텍스트의 경우는 셀렉,언셀렉의 경우 모두 동일하고 백그라운드 컬러 또한 그라디언트가 아닐경우엔 main400 고정으로 보입니다.
@Hyobeen-Park @chattymin
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.
좋은 아이디어인 것 같습니다!
chip이 selected/unSelected 두 상태 모두 배경색과 텍스트 색은 고정이기 때문에 Icon에 해당하는 부분만 서버에서 받아오는게 좋은 방향일 것 같아요. 또한 디자인시스템을 확인해보니 Gradient가 있는 chip과 Soild한 색상의 chip 모두 selected의 background 색상을 제외하곤 전부 같은 상태임을 확인했습니다.
그렇기에 IsGradient라는 매개변수로 구분하여 selelcted일 때의 chip 색상을 결정하는 것은 좋은 아이디어 인 것 같습니다.
이와 관련된 확장함수의 경우 제가 오늘 중으로 구현하여 PR올리도록 하겠습니다 :)
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.
저도 지금은 이 방법이 최선인 것 같아요..!! 다들 어떻게 생각하시나요~~??
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.
저도 좋다고 생각합니다!
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.
해당 방식으로 수정했습니다!
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.
LGTM 🚀
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.
어푸푸~~!!
if (isSelected) { | ||
if (isGradient) { | ||
Modifier.background(color = SpoonyAndroidTheme.colors.black) | ||
} else { | ||
Modifier.background(color = SpoonyAndroidTheme.colors.main400) | ||
} | ||
} else { | ||
Modifier.background(color = SpoonyAndroidTheme.colors.gray0) | ||
} | ||
) |
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.
P3: when문을 써보는건 어떨까요? 가독성이 더 좋아질 것 같아요!
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.
수정했습니다!
@@ -60,76 +69,52 @@ fun IconChip( | |||
) { | |||
AsyncImage( |
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.
P2: 생각해보니까 저희 AsyncImage 래핑해둔게 있더라구요..?? 특별한 이유가 있는 것이 아니라면 해당 컴포넌트 사용해주면 좋을 것 같아요!
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.
수정했습니다!
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.
LGTM~
Related issue 🛠
Work Description ✏️
Screenshot 📸
iconChip_2.mp4
To Reviewers 📢
Background 는 �preview 보시면 modifier.then() 으로 보내시면 됩니다!