Conversation
chattymin
left a comment
There was a problem hiding this comment.
LGTM!
자잘한것들만 수정해주세여~
복잡한 뷰인데 고생했슴다
| searchText = searchText.trim() | ||
| if (searchText.isNotEmpty()) onSearch(searchText) |
There was a problem hiding this comment.
p3
trim하는 이유가 공백만 있을 경우를 검사하는거라면 Blank를 써보세용
글고 얼리리턴으로 Blank이면 리턴시키는 방법은 어떨까요
| searchText = searchText.trim() | |
| if (searchText.isNotEmpty()) onSearch(searchText) | |
| if (searchText.isBlank()) return | |
| onSearch(searchText.trim()) |
| thickness = Dp.Hairline.plus(6.dp), | ||
| color = SpoonyAndroidTheme.colors.gray100 | ||
| ) | ||
| when (tabRowIndex) { |
There was a problem hiding this comment.
p4
여기 밑에가 조금 중복이 많아보이는데 처리할 방법이 있지 않을까용?
한번 생각해보시고 빡세면 패스~
There was a problem hiding this comment.
저 부분 깔끔하게 할 방법을 좀 생각해봤는데 심플하게 경우만 쪼갰을 때 경우의 수가 저 정도라,, 고민이 되네요. 그리고 리뷰 리스트랑 게시물 라스트도 형태가 달라서 그나마 중복이라면 게시물 최근 검색순인지, 유저 최근 검색순인지랑 엠티뷰 정도인데 이거 쪼개면서 어짜피 내부에서는 if나 when문으로 쪼개야 되는거라 그나마 가독성이 제일 좋은 코드로 작성해봤습니다.
| fun switchType( | ||
| searchType: SearchType | ||
| ) { | ||
| _state.update { | ||
| it.copy( | ||
| searchType = searchType, | ||
| searchKeyword = "" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
p4
나중에 type이 늘어날 수 있으니 switchSearchType 어떰?
| searchType: SearchType = SearchType.USER, | ||
| modifier: Modifier = Modifier |
| fun ExploreSearchRecentItem( | ||
| onItemClick: (String) -> Unit, | ||
| onRemoveRecentSearchItem: (String) -> Unit, | ||
| searchKeyword: String | ||
| ) { |
| onSwitchType = { searchType -> viewModel.switchType(searchType) }, | ||
| onRemoveRecentSearchItem = { keyword -> viewModel.removeRecentSearchItem(keyword) }, | ||
| onClearRecentSearchItem = viewModel::clearRecentSearchItem, | ||
| onSearch = { keyword -> viewModel.search(keyword) }, |
There was a problem hiding this comment.
p2. 파라미터 타입 일치하면 직접참조로 바꿔도 됩니다ㅎㅎ
|
|
||
| @Composable | ||
| fun ExploreSearchScreen( | ||
| paddingValues: PaddingValues, |
| LaunchedEffect(Unit) { | ||
| focusRequester.requestFocus() | ||
| } |
There was a problem hiding this comment.
p5 이번에 rememberUpdateState라는걸 알게됐는데 LanchedEffect(Unit or true)같은 곳에 사용하면 좋다고 하네요~
There was a problem hiding this comment.
다들 7차 세미나 자료 정독하고 와라~~ㅋㅋㅋㅋ
| color = SpoonyAndroidTheme.colors.gray700 | ||
| ) | ||
| Icon( | ||
| painter = painterResource(id = R.drawable.ic_close_24), |
Hyobeen-Park
left a comment
There was a problem hiding this comment.
수고하셨습니다~! 3중 when문보고 깜짝 놀랐어요,, 진짜 복잡한데 그래도 잘 하셨네요!!
| onSwitchType = { searchType -> viewModel.switchType(searchType) }, | ||
| onRemoveRecentSearchItem = { keyword -> viewModel.removeRecentSearchItem(keyword) }, | ||
| onClearRecentSearchItem = viewModel::clearRecentSearchItem, | ||
| onSearch = { keyword -> viewModel.search(keyword) }, | ||
| onClearSearchKeyword = viewModel::clearSearchKeyword, |
There was a problem hiding this comment.
| onSwitchType = { searchType -> viewModel.switchType(searchType) }, | |
| onRemoveRecentSearchItem = { keyword -> viewModel.removeRecentSearchItem(keyword) }, | |
| onClearRecentSearchItem = viewModel::clearRecentSearchItem, | |
| onSearch = { keyword -> viewModel.search(keyword) }, | |
| onClearSearchKeyword = viewModel::clearSearchKeyword, | |
| onSwitchType = viewModel::switchType, | |
| onRemoveRecentSearchItem = viewModel::removeRecentSearchItem, | |
| onClearRecentSearchItem = viewModel::clearRecentSearchItem, | |
| onSearch =viewModel::search, | |
| onClearSearchKeyword = viewModel::clearSearchKeyword, |
P3: 하나로 통일해줘잉
| } | ||
|
|
||
| @Composable | ||
| fun ExploreSearchScreen( |
There was a problem hiding this comment.
P1: ExploreSearchRoute에서만 쓰는거니까 private 붙여주세요!
| data class UserInfo( | ||
| val userId: Int = 0, | ||
| val imageUrl: String = "", | ||
| val nickname: String = "", | ||
| val region: String = "" | ||
| ) | ||
|
|
||
| data class PlaceReviewInfo( | ||
| val reviewId: Int = 0, | ||
| val userId: Int = 0, | ||
| val userName: String? = null, | ||
| val userRegion: String? = null, | ||
| val description: String, | ||
| val photoUrlList: ImmutableList<String>? = persistentListOf(), | ||
| val placeName: String? = null, | ||
| val placeAddress: String? = null, | ||
| val category: CategoryEntity? = null, | ||
| val addMapCount: Int? = null, | ||
| val createdAt: String? = null | ||
| ) |
There was a problem hiding this comment.
P1: 이거는 서버에서 받아오는 값 맞죠?!?? state 내부에 만들기보다는 분리해서 model로 만들어주는게 좋을 것 같아요!
There was a problem hiding this comment.
서버 붙이면서 붙여도 만들어도 괜찮을까요!
| enum class SearchType { | ||
| USER, | ||
| REVIEW | ||
| } | ||
|
|
||
| fun SearchType.toKoreanText(): String { | ||
| return when (this) { | ||
| SearchType.USER -> "유저" | ||
| SearchType.REVIEW -> "리뷰" | ||
| } | ||
| } |
There was a problem hiding this comment.
P5: enum 내부에 저장하지 않고 확장함수로 만들어준 이유가 궁금해요!
| val focusRequester = remember { FocusRequester() } | ||
| var tabRowIndex by remember { mutableIntStateOf(0) } | ||
| val tabItems = persistentListOf(SearchType.USER, SearchType.REVIEW) | ||
| var searchText by remember { mutableStateOf("") } |
There was a problem hiding this comment.
P3: searchText를 스크린에서 따로 관리해주는 이유가 궁금합니다! 저는 보통 state에서 관리하는 편이라서요ㅎㅎ
| LaunchedEffect(Unit) { | ||
| focusRequester.requestFocus() | ||
| } |
There was a problem hiding this comment.
다들 7차 세미나 자료 정독하고 와라~~ㅋㅋㅋㅋ
| else -> { | ||
| when (placeReviewInfoList) { | ||
| is UiState.Success -> { | ||
| // 탑색 뷰 컴포넌트 머지 후 구현 예정 |
There was a problem hiding this comment.
P5: 꼭 해야 하는 건 아니지만 이렇게 나중에 해야 할 일을 주석으로 달아둘 때는 TODO: 쓰는게 좋습니다! 안스가 까먹지 않게 알려줘요ㅋㅋ
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun ExploreSearchScreenPreview() { |
There was a problem hiding this comment.
P20: 머지 전에 프리뷰 삭제하실 예정이라고 하셨는데 그러면 PR에 올라오지 않도록 하는게 더 좋지 않을까요??? PR에 불필요한 코드가 많이 올라오면 가독성이 떨어진다고 생각해요!
There was a problem hiding this comment.
Preview 를 파일로 따로 빼두고 PR에는 올리지 않는게 더 좋지 않을까란 말씀인가요?!
음,, 마지막에 지우더라도 Preview 쪽은 무시하고 지나갈 수 있다고 생각하기도 하고 어떤식으로 사용할지 본다면 Preview 가 있는 것도 도움이 된다고는 생각해요.
필요없다고 생각되시면 안보시면 되구(맨 마지막에 Preview 로 어노테이션 적어놓은거라 스킵할 수 있다고 생각합니다.)
만약 어떤식으로 사용되는지 보고 싶으면 해당 부분 코드 확인하실 수 있는게 더 좋지 않나 생각 됩니다! 그리고 다들 Preview 가 코드에 올라가는 거를 별로 안좋아하시는 것 같아서 마지막에 지우긴 할 예정입니다.
바텀 시트나 해당 뷰 같은 경우 저 기준에서 테스트 할 부분이 많아서 코드 리뷰하면서 코드 확인 할겸 남겨둔 것도 있습니다! 따로 확인할 부분이 없으면 계속해서 지우는 편이기도 하고요..!
There was a problem hiding this comment.
파일을 따로 빼도 좋고 코드만 뻬도 좋고~ 방식은 각자 편한 방식으로 하면 될 것 같습니다! 저는 브랜치에 남겨두지 않을 코드는 푸시할 때 제외하려 노력하는 편이라 말씀드려봤습니다😊 그리고 해당 프리뷰는 스크린에 대한 프리뷰라 무겁기도 하구요..!! 컴포넌트의 사용 예시나 UI를 보여주기 위해 Preview를 남기는건 저도 너무 좋아합니다! 하지만 세홍님도 머지 전에 삭제하실 예정이라고 하시기도 했고, 제 생각에는 이 프리뷰는 개발과정에서 확인용으로 만드신 프리뷰인 것 같아서 리뷰 남겼습니다ㅎㅎ 여튼 이건 제 개인 의견이고 이 부분에 대해 정답은 없으니 세홍님의 개발 방식과 맞지 않는다면 그냥 넘기셔도 좋아요😊
| } | ||
|
|
||
| fun search(keyword: String) { | ||
| val type = _state.value.searchType |
There was a problem hiding this comment.
P3: type을 val로 한 번 저장해서 사용하는 이유가 무엇인가요?
There was a problem hiding this comment.
이유를 여쭤보신다면 보기 편해서입니다! when 문이나 if 문에 ..~ 가 들어가는게 가독성이 좋은 것 같지는 않아서 큰 리소스가 없다면 value 에 저장하고 쓰는게 습관이 되었던거같아요..! 그렇지만 성능 최적화를 생각한다면 빼는게 좋을 듯합니다. 수정할게요!
| .noRippleClickable { | ||
| onItemClick(searchKeyword) | ||
| }, |
There was a problem hiding this comment.
P3: 어디선가 리뷰 달았던 것 같긴 한데,,, 람다로 넘기는거랑 함수 그대로 넘기는거 차이점이 무엇일까요?
There was a problem hiding this comment.
이거 searchKeyword 라는 param 보내야돼서
(onclick = onItemClick) 이게 불가능해서 람다에 씌워서 보내긴 했었습니다.. 혹시 좋은 방법 있을까요
There was a problem hiding this comment.
아 그러네요?!?? 이건 인정이지ㅎㅎ 흠 다른 방법을 생각해본다면 searchKeyword도 외부에서 받아오는 값이니까 onItemClick: () -> Unit으로 만들고 외부에서 사용할 때 search(searchKeyword) 이런 식으로 string 넣어주는 방법도 있겠네요! (근데 이거나 그거나 뭐,,, 큰 차이 없을듯 해요ㅎㅎ)
Related issue 🛠
Work Description ✏️
Screenshot 📸
Screen_recording_20250513_205522.mp4
Uncompleted Tasks 😅
To Reviewers 📢
우선 탐색 화면에서 들어가야되는 화면이라 연결을 하지 못해 @Preview 를 넣어뒀어요! 코드 리뷰 확인 후 지우고 머지할 예정입니다.