-
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
[Feat/#9] 장소 상세 페이지 navigation 및 UI 구현 #56
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.
고생하셨습니다! 확인 후 수정 부탁드려요
is UiState.Success -> { | ||
PlaceDetailScreen( | ||
navigateToMap = {}, | ||
menuItems = state.menuItems, | ||
textTitle = state.textTitle, | ||
textContent = state.textContent, | ||
profileUrl = state.profileUrl, | ||
profileName = state.profileName, | ||
profileLocation = state.profileLocation, | ||
dropdownMenuList = state.dropdownMenuList, | ||
imageList = state.imageList, | ||
tag = IconTagEntity( | ||
name = (state.iconTag as UiState.Success<IconTagEntity>).data.name, | ||
backgroundColorHex = (state.iconTag as UiState.Success<IconTagEntity>).data.backgroundColorHex, | ||
textColorHex = (state.iconTag as UiState.Success<IconTagEntity>).data.textColorHex, | ||
iconUrl = (state.iconTag as UiState.Success<IconTagEntity>).data.iconUrl | ||
), | ||
dateString = state.dateString, | ||
locationAddress = state.locationAddress, | ||
locationName = state.locationName, | ||
locationPinCount = state.locationPinCount, | ||
mySpoonCount = state.mySpoonCount | ||
) | ||
} |
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) 매번 검사하기 보다는 이렇게 한번에 처리하는게 좋을 것 같습니다.
p2) Entity
라는 변수명이 presentaion layer에 맞지 않는 네이밍이라 생각돼요. item
, model
등 다른 네이밍으로 수정하는게 좋을 것 같습니다.
is UiState.Success -> { | |
PlaceDetailScreen( | |
navigateToMap = {}, | |
menuItems = state.menuItems, | |
textTitle = state.textTitle, | |
textContent = state.textContent, | |
profileUrl = state.profileUrl, | |
profileName = state.profileName, | |
profileLocation = state.profileLocation, | |
dropdownMenuList = state.dropdownMenuList, | |
imageList = state.imageList, | |
tag = IconTagEntity( | |
name = (state.iconTag as UiState.Success<IconTagEntity>).data.name, | |
backgroundColorHex = (state.iconTag as UiState.Success<IconTagEntity>).data.backgroundColorHex, | |
textColorHex = (state.iconTag as UiState.Success<IconTagEntity>).data.textColorHex, | |
iconUrl = (state.iconTag as UiState.Success<IconTagEntity>).data.iconUrl | |
), | |
dateString = state.dateString, | |
locationAddress = state.locationAddress, | |
locationName = state.locationName, | |
locationPinCount = state.locationPinCount, | |
mySpoonCount = state.mySpoonCount | |
) | |
} | |
is UiState.Success -> { | |
val temp = state.iconTag as UiState.Success<IconTagEntity> | |
PlaceDetailScreen( | |
navigateToMap = {}, | |
menuItems = state.menuItems, | |
textTitle = state.textTitle, | |
textContent = state.textContent, | |
profileUrl = state.profileUrl, | |
profileName = state.profileName, | |
profileLocation = state.profileLocation, | |
dropdownMenuList = state.dropdownMenuList, | |
imageList = state.imageList, | |
tag = IconTagEntity( | |
name = temp.data.name, | |
backgroundColorHex = temp.data.backgroundColorHex, | |
textColorHex = temp.data.textColorHex, | |
iconUrl = temp.data.iconUrl | |
), | |
dateString = state.dateString, | |
locationAddress = state.locationAddress, | |
locationName = state.locationName, | |
locationPinCount = state.locationPinCount, | |
mySpoonCount = state.mySpoonCount | |
) | |
} |
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 = Modifier | ||
.fillMaxSize() | ||
.padding(top = 64.dp) | ||
.verticalScroll(rememberScrollState()) |
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) 추후 scrollState를 활용할 수도 있고, 가독성을 위해서라도 분리해서 변수화해서 넣어주는게 좋을 것 같습니다.
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.
수정했습니다 감사합니다!
Column( | ||
modifier = Modifier | ||
.fillMaxSize() | ||
.padding(top = 64.dp) |
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) Box로 굳이 묶은 후 padding top을 64로 준 이유가 있나요?
TagTopAppBar의 높이 변화를 고려하지 않은 것 같습니다.
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.
수정했습니다!
IconTag( | ||
text = tag.name, | ||
backgroundColorHex = tag.backgroundColorHex, | ||
textColorHex = tag.textColorHex, | ||
iconUrl = tag.iconUrl, | ||
modifier = Modifier.padding(horizontal = 20.dp) | ||
) |
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) 아래 있는 대부분의 요소들이 horizontal 20으로 되어있는데 따로따로 선언한 이유가 없는 것 같아요. 묶어서 한번에 관리해주세요
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.
Column 으로 묶었습니다 감사합니다!
locationSubTitle = locationName, | ||
location = locationAddress | ||
) | ||
Spacer(modifier = Modifier.height(103.dp)) |
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) 이렇게 큰 Spacer가 들어간 이유가 있나요?
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.
구조 변경했습니다! 수정했어요!
var state: MutableStateFlow<PlaceDetailState> = | ||
MutableStateFlow(PlaceDetailState()) | ||
private set |
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) 이 부분에 대한 논의를 해보면 좋을 것 같습니다.
지금처럼 하게 된다면 뷰에서 MutableStateFlow로 캐스팅하여 수정이 가능하다는 단점이 있어요.
반면, MutableStateFlow와 StateFlow를 이용한 backingProperty를 사용한다면 수정을 원천적으로 막을 수 있습니다.
보일러 플레이트가 발생한다는 단점이 있지만, 조금 더 안전하다는 장점이 있어요.
저희팀에서 사용할 방법을 논의해보아요~ @Hyobeen-Park @angryPodo @Roel4990
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 spoonAmount = when (state.spoonAmountEntity) { | ||
is UiState.Success -> (state.spoonAmountEntity as UiState.Success<Int>).data | ||
else -> 0 | ||
} | ||
|
||
val userProfile = when (state.userEntity) { | ||
is UiState.Success -> (state.userEntity as UiState.Success<UserEntity>).data | ||
else -> UserEntity( | ||
userProfileUrl = "", | ||
userName = "", | ||
userRegion = "" | ||
) | ||
} |
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.
p5: state.spoonAmountEntity와 state.userEntity를 처리할 때 UiState를 확인하고 데이터를 추출하는 로직이 중복적으로 사용되는데. 추후에 헬퍼함수로도 추출 해볼수 있을것 같네요~
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.
고생하셨습니다! 수정하고 머지해주세요 :)
.navigationBarsPadding() | ||
.statusBarsPadding() |
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) Scaffold로부터 paddingValue를 받아오는 방법으로 해주세요 :)
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.
수정했습니다!
Spacer(modifier = Modifier.height(27.dp)) | ||
} | ||
PlaceDetailBottomBar( | ||
modifier = 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) 기본값으로 설정해서 매개변수를 넣어주지 않게 해주세요 :)
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.
수고하셨습니다~~! 코리 확인하시고 수정 부탁드릴게요😊
data class IconTagEntity( | ||
val name: String, | ||
val backgroundColorHex: String, | ||
val textColorHex: String, | ||
val iconUrl: String | ||
) |
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: 탐색뷰에서 제작한 CategoryEnttity와 동일한 엔티티인 것 같아서 저희 둘이 하나로 합치면 될 것 같아요!! 그리고 서버 명세서 확인해보시면 categoryId가 필요해서 이건 추가 부탁드려요!!
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.
CategoryEnttity 로 수정했습니다!
text = "떠먹기", | ||
style = ButtonStyle.Secondary, | ||
size = ButtonSize.Medium, | ||
onClick = { onScoopButtonClick() }, |
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.
onClick = { onScoopButtonClick() }, | |
onClick = onScoopButtonClick, |
P2: 짜잔~!
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 userEntity: UiState<UserEntity> = UiState.Loading, | ||
val spoonAmountEntity: UiState<Int> = UiState.Loading, | ||
val scoopDialogVisibility: Boolean = false, | ||
val dropDownMenuList: ImmutableList<String> = immutableListOf("신고하기") |
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: immutableListOf()가 deprecate된 것으로 알고 있는데 한번 더 확인 부탁드릴게요..!!
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.
인자만 얼른 후딱 수정합시다!!!
placeName: String, | ||
spoonAmount: Int, | ||
addMapCount: Int, | ||
isScooped: Boolean = false, |
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: 옵셔널한건 아래로!!
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.
수정했습니다!
|
||
Column( | ||
modifier = Modifier | ||
.width(56.dp) |
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: 오잉? 여기는 왜 고정dp로 되어있나요..?? 특별한 이유가 있나요?!??
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.
minWidth 로 수정했습니다.
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.
어푸가 안돼서,,, 다시 합니다ㅜㅜ
Related issue 🛠
Work Description ✏️
Screenshot 📸
placeDetail.mp4
To Reviewers 📢
더미 데이터 어떤식으로 넣을까 고민하다가 네비게이션 구현한 후 Screen 따로 빼서 네비 구현했습니다. 그래서 조금 더 길어진거 같아요,,