-
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
[PC-190] 매칭 상세 Basic Info Page 디자인 구현 #14
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.
진혁님 고생하셨습니다!
약간의 의견을 코멘트로 남겨보았어요!!
고생하셨습니다~~~~
++
PR명에 PC-190 을 작성해주어야 Jira 이슈 카드와 PR이 연동이 되어요!
그리구 앞으로 커밋 메시지도 PIECE가 아닌 PC로 해주세용! 바뀌었습니다!! 😋
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) ic_bg보다 더 자세한 파일명은 어떨까요? 매칭 상세 화면에서 사용하는 백그라운드니까..
matchingdetail_bg ?!
이 부분 일단은 png로 하고 나중에 직접 canvas에 그려서도 만들 수 있을 것 같아요.
시간나면 해보겠습니다..!
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.
넵! 좋은 것 같아요! 저희 컨벤션 다시 봤는데 이 부분에 대한 컨벤션이 없는 것 같아요! 이번에 리소스 부분에 대한 컨벤션도 정해보면 좋을 것 같아요!
data class MatchingDetailState( | ||
val isLoading: Boolean = false, | ||
) : MavericksState No newline at end of file | ||
val currentPage: MatchingDetailPage = BasicInfoState(), | ||
) : MavericksState { | ||
|
||
sealed class MatchingDetailPage(open val title: String) | ||
|
||
data class BasicInfoState( | ||
override val title: String = "", | ||
val selfDescription: String = "", | ||
val nickName: String = "", | ||
val birthYear: String = "", | ||
val height: String = "", | ||
val religion: String = "", | ||
val occupation: String = "", | ||
val activityRegion: String = "", | ||
val smokeStatue: String = "", | ||
) : MatchingDetailPage(title) | ||
|
||
data class ValueTalkState( | ||
override val title: String = "", | ||
val selfDescription: String = "", | ||
val nickName: String = "", | ||
val talkCards: List<Card> = emptyList(), | ||
) : MatchingDetailPage(title) { | ||
data class Card( | ||
val label: String = "", | ||
val title: String = "", | ||
val content: String = "", | ||
) | ||
} | ||
|
||
data class ValuePickState( | ||
override val title: String = "", | ||
val pickCards: List<Card> = emptyList(), | ||
) : MatchingDetailPage(title) { | ||
data class Card( | ||
val category: String = "", | ||
val question: String = "", | ||
val option1: String = "", | ||
val option2: String = "", | ||
val isSimilarToMe: Boolean = true, | ||
) | ||
} | ||
} |
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) 혹시 이렇게 Page별로 data class를 분리해서 하나의 프로퍼티로 관리하면,
페이지를 넘길 때 마다 데이터를 호출해야하지는 않을까요?
저번이 이야기 나왔던 것이 첫 화면에서 모든 데이터를 한번에 호출하자고 나왔던 것 같아서요!
++
또한,
Page같은 것들을 enum으로 관리하면 다음 페이지로 넘기는 로직을 아래와 같이 하나의 메소드로도 관리할 수 있을 거 같아요!
setJobPostingStep(JobPostingStep.findStep(nowStep + 1))
enum class JobPostingStep(val step: Int) {
TIME_PAYMENT(1),
ADDRESS(2),
CUSTOMER_INFORMATION(3),
CUSTOMER_REQUIREMENT(4),
ADDITIONAL_INFO(5),
SUMMARY(6),
PREVIEW(7),
;
companion object {
fun findStep(step: Int): JobPostingStep {
return JobPostingStep.entries.first { it.step == step }
}
}
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.
- 앗 그렇군요!! 그럼 state를 enum과 페이지의 state로 구성해서 한번에 초기화할 수 있게 수정해두겠습니다!
- 좋은 것 같아요! 페이지에 관련된 비지니스 로직이니 enum에 정의하는 편이 더 좋은 것 같아요! 그리고 한번에 초기화를 해야 한다면 sealed를 못쓰니 enum으로 변경해두긴 해야할 것 같습니다!
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.
추가 코멘트 달았습니다!!!!!!!
😋😋😋
return when (currentPage) { | ||
BasicInfoState -> ValueTalkState | ||
ValueTalkState -> ValuePickState | ||
ValuePickState -> ValuePickState | ||
} | ||
} | ||
|
||
fun getPreviousPage(currentPage: MatchingDetailPage): MatchingDetailPage { | ||
return when (currentPage) { | ||
BasicInfoState -> BasicInfoState | ||
ValueTalkState -> BasicInfoState | ||
ValuePickState -> ValueTalkState | ||
} |
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.
p4) 이렇게 구현한 것도 좋은데,
새로운 페이지가 생긴다거나 페이지 순서가 바뀐다거나 하면 코드 수정이 2군데에서 일어질 것 같아요.
enum옆에 Step을 두고 getNextPage()를 호출하면 현재 페이지에서 +1을 한 enum을 반환하는 것도 좋을 것 같아요.
근데 현재 구현하신대로도 요구사항은 잘 만족하니까 추후에 UI 순서 변동 및 새로운 페이지가 추가될 경우에 수정해도 될 것 같네요 !
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.
태규님이 말씀해주신 것 처럼 Int를 추가할 경우에 화면 전환시 Int 값에 대한 관리가 추가되어서, 그럴 일은 없겠지만... 혹시라도 Int 연산에 오류?!가 생기게 되면 다른 페이지로 이동하게 될 수 있을 것 같아요! 근데 그럴 일은 절대 없을거라 생각합니다. 그것보단 화면 전환 방식에 다른 사항이 고려될 일 없이 명확하게 하는게 좋을 것 같아서 이렇게 작성했습니다! 추후에 개발이 진행되면서 다시 한번 확인해봐요!
val selfDescription: String = "", | ||
val nickName: String = "", | ||
val birthYear: String = "", | ||
val height: String = "", | ||
val religion: String = "", | ||
val occupation: String = "", | ||
val activityRegion: String = "", | ||
val smokeStatue: String = "", | ||
) | ||
|
||
data class ValueTalkState( | ||
val title: String = "", | ||
val selfDescription: String = "", | ||
val nickName: String = "", | ||
val talkCards: List<Card> = emptyList(), | ||
) { | ||
data class Card( | ||
val label: String = "", | ||
val title: String = "", | ||
val content: String = "", | ||
) | ||
} | ||
|
||
data class ValuePickState( | ||
val title: String = "", | ||
val pickCards: List<Card> = emptyList(), | ||
) { | ||
data class Card( | ||
val category: String = "", | ||
val question: String = "", | ||
val option1: String = "", | ||
val option2: String = "", | ||
val isSimilarToMe: Boolean = true, | ||
) | ||
} |
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.
보이는 화면별로 data class로 묶었군요!
혹시 만약 이렇게 구현될 경우 첫 번째 페이지에서 사용하는 데이터와 두 번째 페이지에서 사용하는 데이터가 동일하다면,
어떻게 구현될 수 있나요 ?!
p3) �State가 도메인 모델을 담아야하는데 뭔가 UI에 필요한 데이터 중심으로 설계되는 것 같다는 느낌도 들어요...!
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.
허허 그렇네요 한번에 들고온다면 한 state로 들고와야 할 것 같아요!
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.
앗 이 부분은 flat하게 수정해야될 것 같아요... 수정해놓겠습니다!
Spacer(modifier = Modifier.width(8.dp)) | ||
Button(onClick = onConfirmClick) { | ||
Text("매칭 수락하기") | ||
} | ||
Image( |
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) 한 Layout에 여러 개의 Composable이 있을 경우 사이사이에 개행을 둬서 가독성을 높이는 것도 좋을 것 같아요!
@Preview | ||
@Composable | ||
private fun ProfileValueTalkBodyPreview() { | ||
PieceTheme { | ||
ProfileValueTalkBody() | ||
ProfileValueTalkBody(MatchingDetailState.ValueTalkState()) | ||
} | ||
} | ||
|
||
@Preview | ||
@Composable | ||
private fun ProfileValuePickBodyPreview() { | ||
PieceTheme { | ||
ProfileValuePickBody() | ||
ProfileValuePickBody(MatchingDetailState.ValuePickState()) | ||
} | ||
} | ||
|
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 label: String = "", | ||
val title: String = "", | ||
val content: String = "", | ||
) | ||
|
||
data class ValuePickCard( | ||
val category: String = "", | ||
val question: String = "", | ||
val option1: String = "", | ||
val option2: String = "", | ||
val isSimilarToMe: Boolean = true, | ||
) |
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)
이 부분은 서버 API 호출로 받아오는 부분이라서 Domain 모듈로 이동하면 좋을 것 같아요!
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.
헛 넵! 수정해놓을게여!!
PC-190
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!