[#13] FestivalScreen / MoreScreen 구현 #14
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
*change page size
# Conflicts: # app/src/main/java/kr/ksw/visitkorea/domain/usecase/mapper/ToPresentationModelMapper.kt *cat -> catrgory
f-lab-Toru
reviewed
Oct 20, 2024
| @Query("areaCode") areaCode: String? = null, | ||
| @Query("sigunguCode") sigunguCode: String? = null | ||
| ): ApiResponse<SearchFestivalDTO> | ||
| } |
There was a problem hiding this comment.
프로퍼티명과 파싱할 값의 이름이 동일하다면 굳이 query annotation 을 쓰지 않아도 됩니다.
| eventEndDate: String, | ||
| areaCode: String? = null, | ||
| sigunguCode: String? = null | ||
| ): Result<List<SearchFestivalDTO>> |
| val title: String = "", | ||
| val contentId: String = "", | ||
| val eventStartDate: String = "", | ||
| val eventEndDate: String = "" |
There was a problem hiding this comment.
Trailing comma - 를 추가해 주는 게 좋을 것 같아요.
| TOURIST("12", R.string.tourist_spot_title), | ||
| CULTURE("14", R.string.culture_center_title), | ||
| LEiSURE("28", R.string.leisure_sports_title), | ||
| RESTAURANT("39", R.string.restaurant_title) |
There was a problem hiding this comment.
가능하다면 여기서도 trailing comma 를 추가해 주는 게 어떨까요?
| Surface { | ||
| FestivalCard(Festival( | ||
| "경상북도 칠곡군 동명면 남원리", | ||
| "", |
There was a problem hiding this comment.
네 컴포즈의 Preview를 위한 값입니다.
| } | ||
| ) { index -> | ||
| val festival = festivals[index] | ||
| festival?.run { |
There was a problem hiding this comment.
혹시 festival 이 null 이라면 화면에 어떻게 그려질까요?
There was a problem hiding this comment.
LazyGrid 에서 해당 item은 랜더링 되지 않을것 같습니다.
|
|
||
| import kr.ksw.visitkorea.presentation.common.ContentType | ||
|
|
||
| sealed class HomeUiEffect { |
There was a problem hiding this comment.
HomeUIEffect 가 더 나을 거 같기도 합니다 UI 가 줄임말이기도 하니까요.
| ) | ||
| val data = response.toItems() | ||
| val data = try { | ||
| locationBasedListApi.getLocationBasedListByContentType( |
There was a problem hiding this comment.
try-catch 를 시용했는데, LocationBasedList 가 혹시 Retrofit 의 API 인가요 아니면 다른 레이어인가요? 제가 봤을때는 예외처리가 이미 완료된 데이터가 사용되는게 더 나을 거 같거든요. 같이 이야기해 보시죠.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
변경사항
멘토님께
동작화면
Screen_recording_20241017_135451.mp4
Screen_recording_20241017_135528.mp4