[PC-54] 매칭 상세 페이지 화면 전환 구현 #10
Conversation
sksowk156
added
UI/UX 🎨
There was a problem hiding this comment.
진혁님 수고하셨습니다!
큰 변동사항은 없고 네이밍과 컨벤션 관련 코멘트가 많아요!
하나씩 차근차근 읽어봐주세용~~~~
그리고 이건 조금 이해가 안되는데 local properties가 왜 자꾸 반영되는 걸까요?! gitignore에도 등록되어 있는데,,, 제가 뭘 잘못한걸까요?!
흠... 저도... 잘... ㅠㅠㅠㅠㅠㅠㅠㅠ 커밋하구 그럼 따로 local properties를 commit stage에서 일일이 내려주시는 것이 어떨까요 ㅋㅌㅋㅌㅋ?
git ignore에 여러가지 넣어봐야할 듯...
| if (currentDestination?.isHideBottomNavigationRoute() == false) { | ||
| if (currentDestination?.shouldHideBottomNavigation() == false) { |
There was a problem hiding this comment.
shouldXXX 네이밍이 더 자연스러운 것 같아요! 구웃
| /** | ||
| * 숨겨야 하는 경로들을 상수로 정의 | ||
| */ |
There was a problem hiding this comment.
주석이 없어도 코드가 잘 읽히는 것 같아요!
다만, 해당 프로퍼티의 위치를 shouldHideBottomNavigation() 주위로 두면 더 가독성이 좋을 것 같네요.
There was a problem hiding this comment.
아하 넵 알겠습니다! 저도 모르게 프로퍼티를 제일 위로 올려버렸네요
| ) { | ||
| val pageIndex = remember { mutableIntStateOf(0) } |
There was a problem hiding this comment.
p2) 제 생각에 이 부분은 remember보다는 rememberSaveable을 사용하는 것이 더 좋을 것 같아요.
그리고 by 연산자를 사용해서 pageIndex.IntValue 같은 보일러 플레이트를 제거하는 것은 어떨까요?
p4) 그리고 해당 값을 ViewModel로 올리는 것에 대해서는 어떻게 생각하시나요 ?!
There was a problem hiding this comment.
- 아 사실 이번 PR은 화면 전환 구현이 주목적이여서 내부의 디테일한 ui 부분은 임시로 구현해놨습니다! 그래서 세부적인 요소는 신경쓰지 않고 구현했는데, 이번 PR이 끝나는데로 다시 한 페이지씩 디자인을 적용해보겠습니다!
- 넵 좋은 생각인 것 같아요! state로 정의해서 viewmodel로 관리하면 좋을 것 같아요!
| when (pageIndex.intValue) { | ||
| 0 -> ProfileBasicInfoBody() | ||
| 1 -> ProfileBasicValueBody() | ||
| 2 -> ProfileAllValueBody() | ||
| } |
There was a problem hiding this comment.
p3) 그냥 MagicNumber보다 enum class를 사용해서 상수화 시키면 좋을 것 같네요!
| if (pageIndex.intValue > 0) pageIndex.intValue-- | ||
| }, | ||
| onCloseClick = onCloseClick, | ||
| title = "가치관 pick" |
There was a problem hiding this comment.
p3) 이 부분 혹시 나중에 다국화를 고려하여 string.xml으로 분리해주실 수 있을까요?
There was a problem hiding this comment.
넵 이 부분도 이번 PR 끝나고 ui 작업하는 데로 말씀하신 부분도 적용해둘게요!
There was a problem hiding this comment.
이 부분 일단 머지하고 다음 PR에서 ViewModel로 올린 뒤 enum으로 꼭 바꿔주세용~~
| items(dummyItems) { item -> | ||
| BasicValueCard(item) | ||
| } |
There was a problem hiding this comment.
p2) 지금은 dummy이지만 나중에는 key를 반드시 사용하면 좋을 것 같아요!
|
|
||
| @OptIn(ExperimentalLayoutApi::class) | ||
| @Composable | ||
| fun ProfileBasicInfoBody() { |
There was a problem hiding this comment.
p4) 아래 화면인 것 같은데, 가치관 Talk이라는 타이틀에 맞게 네이밍을 지으면 더 좋을 것 같아요!
There was a problem hiding this comment.
앗 제가 잘못봤군요! 가치관 Talk이었군요!
| data class BasicValueItem(val title: String, val description: String) | ||
|
|
||
| fun dummyBasicValueItems() = listOf( | ||
| BasicValueItem("음악 취향", "여행하며 모을 감성, LGBTQ+ 권리를 말해요..."), | ||
| BasicValueItem("공통 가치관", "요리, 고기, 라이딩 좋아해요..."), | ||
| BasicValueItem("연애관", "서로 존중하고 신뢰받으며 성장하는 관계...") | ||
| ) |
There was a problem hiding this comment.
p3) 이 부분도 더미용으로 UI 레이어에 데이터 클래스가 있는 것이겠죠?
domain 레이어에 작성하고 ViewModel에서 꺼내서 써야할 것 같아요!
| @Composable | ||
| fun ProfileAllValueBody() { |
There was a problem hiding this comment.
p3) 이 부분은 가치관 Pick 이라는 타이틀이 있으니 그에 맞는 네이밍을 가져가면 더 직관적일 것 같네요.
There was a problem hiding this comment.
사실 요 부분 네이밍에 좀 고민했는데, 여기 페이지가
이거랑
이렇게 2개가 있어서 ProfileBasicValueBody 랑 ProfileAllValueBody로 구분했는데, 어떻게 수정하면 좋을까요?!
There was a problem hiding this comment.
제가 타이틀을 잘못 봐서 2개 타이틀이 같은 가치관 pick인줄 착각했습니다! 😭
ProfileValueTalkBody, ProfileValuePickBody 로 변경하려고 하는데 어떻게 생각하시나요?
There was a problem hiding this comment.
제가 타이틀을 잘못 봐서 2개 타이틀이 같은 가치관 pick인줄 착각했습니다! 😭 ProfileValueTalkBody, ProfileValuePickBody 로 변경하려고 하는데 어떻게 생각하시나요?
좋은 것 같습니다!
| } | ||
| } | ||
|
|
||
| private fun processOnMatchingDetailCloseClickIntent() { |
There was a problem hiding this comment.
MVI를 사용할 때에는 navigationHelper를 private으로 두고 Intent로 이벤트를 받은 뒤 ViewModel 내부에서만 핸들링하면 되겠군요!
tgyuuAn
added
머지 해도될듯염🌟
PC-54
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!