Conversation
Collaborator
There was a problem hiding this comment.
수고하셨습니다! @@@꼭 다른주차 피드백 확인해주세요!@@@
pr내용은 꼭 적으셔야 합니다
전체적으로 ListAdapter를 상속받으면서도 내부 리스트를 수동으로 관리하거나 notifyDataSetChanged()를 사용하는 등 라이브러리의 의도와 다르게 작성된 부분들이 보입니다...
또한, 데이터 모델 변환 로직이 프래그먼트에 직접 노출되어 있어 유지보수 효율이 떨어질 수 있으니, 매퍼도입이나 리포지토리 로직 개선을 통해 프래그먼트의 책임을 줄여보시면 좋을 것 같습니다. 사소한 버그(Activity/Fragment 혼용)와 하드코딩된 문자열들만 정리하면 훨씬 수준 높은 코드가 될 것 같습니다
고생 많으셨어요!
Comment on lines
+12
to
+34
| class ProductDetailActivity : Fragment() { | ||
|
|
||
| override fun onCreateView( | ||
| inflater: LayoutInflater, | ||
| container: ViewGroup?, | ||
| savedInstanceState: Bundle? | ||
| ): View? { | ||
| val view = inflater.inflate(R.layout.fragment_product_detail, container, false) | ||
|
|
||
| val detailImage = view.findViewById<ImageView>(R.id.detailImage) | ||
| val detailName = view.findViewById<TextView>(R.id.detailName) | ||
| val detailPrice = view.findViewById<TextView>(R.id.detailPrice) | ||
|
|
||
| val name = arguments?.getString("name") ?: "" | ||
| val price = arguments?.getString("price") ?: "" | ||
| val imageResId = arguments?.getInt("image") ?: R.drawable.home_banner | ||
|
|
||
| detailImage.setImageResource(imageResId) | ||
| detailName.text = name | ||
| detailPrice.text = price | ||
|
|
||
| return view | ||
| } |
Collaborator
There was a problem hiding this comment.
ProductDetailActivity.kt 파일에서 이름은 Activity이지만 실제로는 Fragment를 상속받고 있습니다. HomeFragment에서는 이를 Intent로 실행하려 하고 있어 런타임 에러가 발생할 것으로 보입니다. 실제 Activity로 선언하거나, Navigation Component를 활용해 Fragment 간 이동으로 통일해야 합니다..
Comment on lines
+17
to
+25
| inner class ProductViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) { | ||
| val imgProduct: ImageView = itemView.findViewById(R.id.imgProduct) | ||
| val imgLike: ImageView = itemView.findViewById(R.id.imgLike) | ||
| val tvBestSeller: TextView = itemView.findViewById(R.id.tvBestSeller) | ||
| val tvName: TextView = itemView.findViewById(R.id.tvName) | ||
| val tvDescription: TextView = itemView.findViewById(R.id.tvDescription) | ||
| val tvColorCount: TextView = itemView.findViewById(R.id.tvColorCount) | ||
| val tvPrice: TextView = itemView.findViewById(R.id.tvPrice) | ||
| } |
Collaborator
There was a problem hiding this comment.
findviewByid방식 뷰바인딩으로 꼭 바꿔주세요!!!!
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
📌 PR 제목
🔗 관련 이슈
Closes #이슈번호
✨ 변경 사항
🔍 테스트
📸 스크린샷 (선택)
🚨 추가 이슈