Skip to content
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

[#17] 상세화면 (DetailScreen) 구현 #18

Merged
merged 6 commits into from
Oct 26, 2024
Merged

[#17] 상세화면 (DetailScreen) 구현 #18

merged 6 commits into from
Oct 26, 2024

Conversation

ksw4015
Copy link
Collaborator

@ksw4015 ksw4015 commented Oct 24, 2024

변경사항

  • 상세정보 검색 API(공통, 소개, 반복, 이미지)와 DTO, Repository가 추가되었습니다.
  • ContentTypeId를 상수 처리하였습니다.
  • 상세화면에서 사용할 UseCase가 추가되었습니다.
  • domain의 Mapper가 수정되었습니다.
  • DetailActivity, Screen, ViewModel이 추가되고, Intent로 전달하기 위한 직렬화 class인 DetailParcel이 추가되었습니다.
  • HomeScreen의 관광지 Item에 Click Event가 추가되었습니다.

동작화면

Screen_recording_20241024_135309.mp4

*add constants
*add detail module
(cherry picked from commit e592316)
*change type of parameter in detail api
*add Parcelize plugin
*add Parcelize annotation to DetailParcel
(cherry picked from commit 929867d)
@ksw4015 ksw4015 added the detail common detail screen develop label Oct 24, 2024
@ksw4015 ksw4015 requested a review from f-lab-Toru October 24, 2024 04:57
@ksw4015 ksw4015 self-assigned this Oct 24, 2024
Copy link

@f-lab-Toru f-lab-Toru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고 많으셨습니다. 코멘트롤 몇 개 달아놨는데 간단한 것들은 바로 처리할 수 있으실 거라 생각되구요, 그 외 replace 가능한 것들에 대한 것도 한번 생각해 보세요

@Query("contentId") contentId: String,
@Query("contentTypeId") contentTypeId: String
): ApiResponse<DetailIntroDTO>
/*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안 쓸 코드는 주석처리 대신에 없애는 게 좋을 거 같아요.


data class DetailCommonDTO(
val homepage: String,
val overview: String

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma 를 넣어도 될 거 같습니다

@Query("contentTypeId") contentTypeId: Int
)
*/
@GET("detailImage1")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

방금 든 생각인데, URL 을 const 로 정의해서 써도 될 거 같습니다

@SerializedName("subfacility")
val subFacility: String?,
@SerializedName("reservationurl")
val reservationUrl: String?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma를 넣어도 될 거 같습니다.
그리고 만일의 경우를 대비해 초기값을 넣어도 될 거 같아요

val restDate: String? = null,
val fee: String? = null,
val menus: String? = null,
val place: String? = null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma 를 넣어 주는 게 좋을 거 같습니다

.data(detailState.firstImage)
.size(Size.ORIGINAL)
.build(),
colorFilter = if(detailState.firstImage.isNotEmpty())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if-else 를 사용할 때 {} 를 적절히 써 주면 가독성에 도움이 될 거 같기도 합니다

val contentTypeId: String = "",
val detailCommon: DetailCommonDTO = DetailCommonDTO("",""),
val detailIntro: CommonDetail = CommonDetail(),
val images: List<DetailImageDTO> = emptyList()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma 를 넣어 주는 게 좋을 거 같아요

)
getDetailImage(
detailParcel.contentId,
if(detailParcel.contentTypeId == TYPE_RESTAURANT)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if - else 에 {} 를 적절히 써 주는 게 좋을 거 같아요


@Provides
@Singleton
fun provideDetailRepository(detailApi: DetailApi): DetailRepository {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

방금 든 생각인데, 이 부분도 @Binds 를 이용해서 interface 를 리턴할 수 있도록 할 수 있지 않을까 하는 생각이 드네요.

@ksw4015 ksw4015 merged commit 3101fa9 into main Oct 26, 2024
1 check failed
@ksw4015 ksw4015 deleted the feature/17 branch October 29, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
detail common detail screen develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants