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

✨ 전체 뱃지 페이지 UI 구현 #86

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Conversation

peter-j0y
Copy link
Member

😎 작업 내용

  • 전체 뱃지 페이지 UI 구현
  • WalkieTitleBar 컴포넌트 만들어놓았으니 이용하시면 됩니다
  • 빠른 작업을 위해 코드 겁나 더럽습니다 ㅋㅋ

🥳 동작 화면

badge_page_ui.mp4

@peter-j0y peter-j0y self-assigned this Sep 8, 2024
Copy link
Member

@soopeach soopeach 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 +18 to +22
@Composable
fun WalkieNormalButton(
buttonText: String,
onButtonClick: () -> Unit,
) {
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.

제가 확인했을 때는 없었던거 같아서 만들었어요. 당시 기억이 잘 안나네요 ㅎㅎ

) {
var currentPosition by remember { mutableStateOf(Offset.Zero) }
var isDragging by remember { mutableStateOf(false) }
Log.d("sm.shin", "dragtarget: $dataToDrop")
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.

로그가 여기저기 흩어져 있으면 한번에 보기 힘들어서 그냥 뭐로 하지 하다가 이름으로 쓰는거 같아요. 저는 팀장님이 그렇게 하길래 간지나보여서 따라하다보니 습관이 돼서ㅋㅋ

Comment on lines +60 to +72
@Preview
@Composable
private fun BadgePreview() {
WalkieTheme {
BadgeItem(
BadgeInfo(
1,
R.drawable.badge_test_2,
"test"
)
)
}
}
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.

말씀하신 것처럼 협업을 위한 용도 + 다양한 엣지 케이스나 글자 크기 테스트가 필요한 경우에 씁니다. 보통 디자인 가이드는 글자 크기와 1줄인 것, 여러 줄인 것 모두 같이 나오니까 빌드마다 바꿔서 실행하기 힘드니까요.

Comment on lines +141 to +147
class DragTargetInfo {
var isDragging: Boolean by mutableStateOf(false)
var dragPosition by mutableStateOf(Offset.Zero)
var dragOffset by mutableStateOf(Offset.Zero)
var draggableComposable by mutableStateOf<(@Composable () -> Unit)?>(null)
var dataToDrop by mutableStateOf<Any?>(null)
}
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.

ui 관련된 state들을 모아놓는 state holder의 개념이라 추후에 ui logic이 추가될 수도 있을 것 같아 class로 했어요. 지금 상황에서는 data class로 해도 상관 없을 거 같긴 합니다!

Copy link
Member

Choose a reason for hiding this comment

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

@peter-j0y ui Logic이라 함은 정확이 어떤 의미일까요???!!
간단한 함수의 동작(?)으로 이해했는데, 데이터 클래스에서도 함수는 만들 수 있지 않은가요??

Copy link
Member Author

@peter-j0y peter-j0y Sep 12, 2024

Choose a reason for hiding this comment

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

@soopeach
state holder에서의 로직은 보통 holder안에 있는 state들을 가지고 하는 로직이니까 말씀하신대로 간단한 로직일 가능성이 높긴하다고 생각합니다. 여기서는 drag position이 특정 position일 경우 ui가 변해야하는 정책 같은게 있다면 그런 ui logic 함수가 추가될 수 있을 것 같아요.

data class에서도 함수를 만들 수 있어서 선택의 문제이지만 개인적으로 함수나 로직이 들어가는 data class 사용은 지양하는 편입니다. data class는 말그대로 data를 주고 받을 때 사용하는 객체이고 불변을 유지하는 데에 특화되어 있어 이러한 특성을 지키는게 좋다고 생각해요. data class에서 copy함수나 equal toString같은 데이터를 다루고 디버깅하기 편한 함수를 자동으로 override해서 만들어주는 것도 동일한 맥락이라고 생각해요. drag에 대한 ui 정보가 계속 변하는 경우와 같은 지금 상황에서 불변을 유지하기 위해 copy()를 통해 계속 값을 바꿔주면 오히려 객체가 너무 많이 생겨 비효율적일 것 같고 var로 값을 계속 바꾸는 것은 data class의 의도에서 약간 벗어나지 않나 싶었습니다.

@peter-j0y peter-j0y merged commit 3e61023 into compose/develop Sep 12, 2024
1 check passed
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.

2 participants