-
Notifications
You must be signed in to change notification settings - Fork 72
[하티] 3, 4단계 쇼핑 장바구니 제출합니다. #49
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
Conversation
- 수량 증가 및 감소
- 가격 관련 포맷팅 간단하게 수정
- 삭제, 수량 변경 시에도 가격이 업데이트된다
- 아이템을 삭제했을 때 하단 페이지 버튼의 업데이트가 제대로 이루어지도록 수정
- db 테이블 수정 - repository 수정 - presenter 테스트 수정
galcyurio
left a comment
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.
미션 진행하느라 고생하셨습니다. 👏👏
모든 요구사항이 충족된게 아니라서 질문주신 부분들과 MVP 패턴을 위주로 피드백을 남겼습니다.
이번 미션 끝까지 힘내세요! 화이팅!
app/src/main/java/woowacourse/shopping/ui/cart/adapter/CartListAdapter.kt
Show resolved
Hide resolved
app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailPresenter.kt
Show resolved
Hide resolved
|
안녕하세요 토리! 다른 미션 진행에 바빠 피드백 반영이 늦어졌네요 ㅠㅠ 다만, 커밋 refactor: BindingAdapter 적용 이후로 3개의 커밋을 더 보냈었는데, 마지막 피드백도 잘 부탁드려요 🌸 |
galcyurio
left a comment
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.
마지막까지 고생하셨습니다. 👏👏
추가로 몇 가지 코멘트를 남겨두었으니 확인해주세요!
반영할 피드백이 많지 않아서 이번 미션은 여기서 마무리하겠습니다.
방학 잘 보내시길 바랄게요!
| @Test | ||
| fun 앱_첫_실행_후_전체_상품_리사이클러뷰가_보인다() { | ||
| 해당_항목이_보인다(R.id.rv_main_product) | ||
| } | ||
|
|
||
| @Test | ||
| fun 앱_첫_실행_후_최근_본_상품_리사이클러뷰가_보이지_않는다() { | ||
| 해당_항목이_보이지_않는다(R.id.rv_recently_viewed) | ||
| } | ||
|
|
||
| @Test | ||
| fun 앱_첫_실행_후_상품_한_개를_보면_최근_본_상품_리사이클러뷰가_보인다() { | ||
| 해당_항목이_보이지_않는다(R.id.rv_recently_viewed) | ||
|
|
||
| 상품_리스트의_해당_아이템을_클릭하면(1) | ||
| 상세_페이지에서_뒤로가기를_누르면() | ||
|
|
||
| 해당_항목이_보인다(R.id.rv_recently_viewed) | ||
| } | ||
|
|
||
| @Test | ||
| fun 상품_목록_스크롤을_올리면_최근_본_상품_리사이클러뷰가_보이지_않는다() { | ||
| 상품_리스트의_해당_아이템을_클릭하면(1) | ||
| 상세_페이지에서_뒤로가기를_누르면() | ||
|
|
||
| 해당_항목이_보인다(R.id.rv_recently_viewed) | ||
|
|
||
| 화면의_스크롤을_내리면() | ||
|
|
||
| 해당_항목이_보이지_않는다(R.id.rv_recently_viewed) | ||
| } |
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.
모든 동작을 한글로 된 함수로 만들어주셔서 가독성이 엄청 좋네요 👍️
| override fun onPlusCountButtonClick(productId: Long, oldCount: Int) | ||
| override fun onMinusCountButtonClick(productId: Long, oldCount: Int) | ||
| fun onCloseButtonClick(productId: Long) | ||
| fun onCheckboxClick(isChecked: Boolean, item: CartUIState) |
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.
여러 콜백을 하나로 잘 묶어주셨네요 👍️
| import com.bumptech.glide.load.engine.DiskCacheStrategy | ||
| import woowacourse.shopping.R | ||
|
|
||
| @BindingAdapter("ImageBinding") |
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.
안드로이드의 기본 속성을 그대로 사용하실 수도 있습니다.
@BindingAdapter("android:src")| android:layout_marginHorizontal="18dp" | ||
| android:layout_marginBottom="20dp" | ||
| android:background="@drawable/shape_border_grey_1dp" | ||
| android:onClick="@{()->onRecentProductClick.invoke(recent_product.id)}" |
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 binding을 활용할 때도 kotlin, java의 coding convention을 그대로 따라 주시는게 좋습니다.
| android:onClick="@{()->onRecentProductClick.invoke(recent_product.id)}" | |
| android:onClick="@{() -> onRecentProductClick.invoke(recentProduct.id)}" |
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.
id를 snake_case로 지어서 그러신 것 같은데 tv_cart_count와 같이 지어진 ID도 데이터바인딩을 통해 생성된 코드에서는 tvCartCount와 같이 변경됩니다.
| <variable | ||
| name="onItemClick" | ||
| type="kotlin.jvm.functions.Function1" /> |
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.
Function1를 넘기기보다는 다른 곳처럼 별도 인터페이스로 선언해주시는게 좋습니다.
그 이유는 매개변수가 늘어나거나 줄어드는 경우 type도 변경되어야하기 때문이에요.
<variable
name="listener"
type="woowacourse.shopping.listener.ProductItemListener" />
안녕하세요, 토리!
주말동안 잘 지내셨나요? 😄
우선 1, 2단계 때 주신 피드백들에 매우 감사드려요 :)
피드백들 반영하고 3, 4단계 미션도 열심히 해보았습니다!
다만, 테코톡 등 다른 일들과 병행하여 진행하게 되면서 모든 시간을 투자하지 못했고
그에 따라 일부 기능 구현이 미완성된 상태입니다 ㅠㅠ
하지만 우선 리뷰를 받고 싶어서 현재까지의 구현들에 대해 PR을 보냅니다..!
남은 기능들에 대해서는 다음 피드백 반영하는 것과 함께 제출할 수 있도록 할게요!
이번 3, 4 단계에서도 성능과 구조적인 부분에 대한 고민을 하면서 진행한 것 같습니다.
view에 관련해서는 재사용 (특히 리사이클러뷰와 관련하여) 측면에서 잘 이루어지고 있는지가 주된 고민이었고,
리사이클러뷰와 관련하여 어댑터, 뷰 홀더가 적절한 역할을 수행하여 잘 진행이 되고 있는지가 궁금해요.
또한, 데이터베이스에 접근하는 과정을 최소화하기 위해 로컬 저장을 이용하려고 했는데,
어떤 시점에서 로컬 저장을 활용하고, 어떤 시점에서 db와 동기화를 하는지, 와 같은 부분들이 혼란스러웠어요.
매번 db에서 쿼리문을 실행하고, 혹은 db에서 모든 아이템을 가져오는 로직을 실행하는 것이 성능적으로 부담이 적지 않을 것 같은데,
그렇다면 미리 아이템을 로컬에 저장해두고 계속해서 사용하면 좋지 않을까? 하는 고민이 들었거든요.
이런 경우에, 어떤 식으로 플로우가 많이 이뤄지는지, 어떠한 trade-off가 있을지 토리의 의견이 궁금해요!
그 외에도 이번에도 미션과 관련하여 다른 피드백들이 있으시다면 많이 알려주세요! 잘 부탁드립니다!! 🌸