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

Seminar2 #2

Merged
merged 14 commits into from
May 2, 2022
Merged

Seminar2 #2

merged 14 commits into from
May 2, 2022

Conversation

papajj06
Copy link
Member

@papajj06 papajj06 commented Apr 21, 2022

Related Issues

close #4

  • level1 필수과제
  • level2 성장과제
  • level3 도전과제

Copy link

@l2hyunwoo l2hyunwoo left a comment

Choose a reason for hiding this comment

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

수고하셨습니다

val paddingLeft: Int,
val paddingRight: Int
) : ItemDecoration() {
private var mDivider: Drawable? = null

Choose a reason for hiding this comment

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

코틀린에서 멤버변수는 m prefix 제거하는게 좋습니다~

Comment on lines +17 to +18
outRect.left = divHeight
outRect.right = divHeight

Choose a reason for hiding this comment

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

outRect의 left, right 속성과 divHeight의 관계가 어떻게 될까? 만약 패딩 크기만을 받아온 것이라면 padding or paddingHorizontal 정도로 이름 잡아도 좋을 듯

Copy link
Member Author

Choose a reason for hiding this comment

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

오호,,!! 공부하겠습니다아!

Comment on lines +41 to +57
companion object {
val DIFFUTIL = object : DiffUtil.ItemCallback<FollowerData>() {
override fun areItemsTheSame(
oldItem: FollowerData,
newItem: FollowerData
): Boolean {
return oldItem.name == newItem.name
}

override fun areContentsTheSame(
oldItem: FollowerData,
newItem: FollowerData
): Boolean {
return oldItem == newItem
}
}
}

Choose a reason for hiding this comment

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

GOOD 👍🏻

Choose a reason for hiding this comment

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

좋습니다~

Copy link
Member Author

Choose a reason for hiding this comment

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

모두 제 스승님들 덕분입니다 (꾸벅)🤟😆

Comment on lines +74 to +76
fun afterDragAndDrop() {
notifyDataSetChanged()
}

Choose a reason for hiding this comment

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

이 친구는 그냥 private으로 선언해도 될 것 같음

Copy link
Member Author

Choose a reason for hiding this comment

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

넵!! 확인요~

Comment on lines +60 to +62
interface OnStartDragListener {
fun onStartDrag(viewHolder: FollowerViewHolder)
}

Choose a reason for hiding this comment

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

인터페이스에 함수가 하나만 있으면 fun interface 사용하는 것도 좋을 듯

Choose a reason for hiding this comment

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

새로운 배움 한 접시 떠먹고 갑니다.

Choose a reason for hiding this comment

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

필린에서 알려주셨는데 리마인드 합시다.

Copy link
Member Author

Choose a reason for hiding this comment

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

오호,,!! 이제야 작은 것들 하나하나가 눈에 보이네요,,, 감삼댜 반영하겠습니다~

Comment on lines +84 to +87
with(binding) {
rvFollower.addItemDecoration(VerticalItemDecorator(10))
rvFollower.addItemDecoration(HorizontalItemDecorator(10))
}

Choose a reason for hiding this comment

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

10은 px단위로 들어가니 너가 원하는 단위(dp)로 들어가게 하려면 dp -> px 변환식 사용해서 넣어야할 것 같음

Choose a reason for hiding this comment

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

저는 이거 Utili로 따로 빼서 확장함수로다가 만들어 씁니다 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다~!~! 이 부분 또한 공부하겠습니당

Comment on lines 51 to 66
// interface OnStartDragListener {
// fun onStartDrag(viewHolder: RepoAdapter.RepoViewHolder)
// }
//
// override fun onItemMove(fromPosition: Int, toPosition: Int) {
// Collections.swap(repoList, fromPosition, toPosition)
// notifyItemMoved(fromPosition, toPosition)
// }
//
// override fun onItemSwipe(position: Int) {
// repoList.removeAt(position)
// notifyItemRemoved(position)
// }
//
// fun afterDragAndDrop() {
// notifyDataSetChanged()

Choose a reason for hiding this comment

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

필요없는 부분은 주석 지워주세요

Comment on lines 20 to 22
override fun onDestroy() {
super.onDestroy()
}

Choose a reason for hiding this comment

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

액티비티에서는 굳이 필요없는 것 같음

initAdapter()
}

abstract fun initAdapter()

Choose a reason for hiding this comment

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

이게 모든 프래그먼트에서 필요한 로직일까? 프래그먼트가 공통적으로 가져야할 로직들를 추상화한 것이 베이스 클래스인걸 한번 염두해두고 설계하는게 좋을 것 같아

Copy link
Member Author

Choose a reason for hiding this comment

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

흠~~ 넵!! 2주차 과제만 생각할 때 공통인 부분을 최대한 넘길라고 했어서 여기 베이스 클래스에 쏘옥 집어 넣은 것입니다요~!
이 부분 반영하겠습니다!!

@papajj06
Copy link
Member Author

수고하셨습니다

더 코드리듀 뷰탁드리영

Copy link

@hansh0101 hansh0101 left a comment

Choose a reason for hiding this comment

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

뒤풀이에서의 팩트폭행을 사과드리며 리뷰 남깁니다. 수고하셨습니다.


class HorizontalItemDecorator(private val divHeight: Int) : RecyclerView.ItemDecoration() {

@Override

Choose a reason for hiding this comment

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

@Override 어노테이션을 붙인 이유가 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

구글 선생님거,,, 긁어 와버렸,,,, 사실,,이유가 없써요,,😂

import android.view.View
import androidx.recyclerview.widget.RecyclerView

class VerticalItemDecorator(private val divHeight: Int) : RecyclerView.ItemDecoration() {

Choose a reason for hiding this comment

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

저는 그냥 MyItemDecoration 클래스 하나 만들고 구분선이랑 상하좌우 offset 다 한 번에 적용했는데 이렇게 분리해서 쓰면 더 좋을수도 있겠다는 배움을 얻고 갑니다. 👍

}
}

inner class FollowerViewHolder(

Choose a reason for hiding this comment

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

inner class로 만든 이유가 궁금합니다. (본인 29기 앱잼 때 멘토님께 지적들음 ㅜ)

Choose a reason for hiding this comment

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

필린에서도 말씀드렸는데 22
리마인드 합시다.22

Copy link
Member Author

Choose a reason for hiding this comment

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

my mistake,,, 기억납니다,, inner,,, 앞으로 명심하겠습니다요!!!!!!

Comment on lines +60 to +62
interface OnStartDragListener {
fun onStartDrag(viewHolder: FollowerViewHolder)
}

Choose a reason for hiding this comment

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

새로운 배움 한 접시 떠먹고 갑니다.

}

override fun onItemMove(fromPosition: Int, toPosition: Int) {
Collections.swap(currentList, fromPosition, toPosition)

Choose a reason for hiding this comment

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

이번 과제를 하면서 가장 큰 고민을 했던 부분이었습니다. LinearLayout의 RecyclerView에서는 고민 없이 swap을 사용했는데, spanCount = 2인 GridLayout의 경우

0번 아이템 & 1번 아이템
2번 아이템 & 3번 아이템
4번 아이템 & 5번 아이템
...

이런 식으로 아이템들이 그려지게 될텐데, 무작정 swap을 사용하게 될 경우 0번 아이템을 드래그해서 2번 아이템 위치로 드래그할 때 문제가 생기게 되더군요... notifyItemMoved를 사용하니 겉보기에는 그저 0번 아이템이 2번 아이템 자리로 이동하고, 1번 아이템과 2번 아이템이 한 칸씩 앞으로 당겨지도록 뷰가 다시 그려졌으나 실제 swap의 결과로 인해 List에서는 [2번, 1번, 0번, 3번, 4번, ...] 의 형태로 교체되어 애를 먹었습니다. 수빈님도 이런 부분을 한 번 고민해보면 성장에 도움이 되지 않을까 싶습니다.

Choose a reason for hiding this comment

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

Grid로 RecyclerView를 구성한 RepoFragment에는 아이템 이동 삭제 구현이 주석처리로 빠져있길래 남겨봤습니다. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

wow,,, 감사합니다 깊게 생각을 안해봐서,,,반성하겠습니다. 이해가 쏙쏙 되네요. 공부하겠습니다아!!!

Comment on lines +12 to +13
private var _binding: B? = null
val binding get() = _binding!!

Choose a reason for hiding this comment

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

BaseActivity에서 nullable한 binding 객체 _binding과 get()으로 _binding을 가져오는 not null한 binding 객체 binding을 분리해서 사용하는 이유가 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

구글,,선생님꺼를 빌려오다보니,, 이런 일이 생겼습니다...공부하겠습니다,,:)


override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
binding.lifecycleOwner = this

Choose a reason for hiding this comment

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

BaseActivity에서는 lifecycleOwner 코드를 추가하지 않았는데 거기도 추가해주면 좋지 않을까요? 저도 잘 모름

if (isMoved) {
isMoved = false
val followAdapter = FollowerAdapter()
followAdapter.afterDragAndDrop()

Choose a reason for hiding this comment

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

afterDragAndDrop() 메서드 내부는 nofityDataSetChanged() 한 줄만 있으니, 현우님 리뷰처럼 private으로 돌리거나 저라면 그냥 이렇게 할 것 같습니다.

Suggested change
followAdapter.afterDragAndDrop()
followAdapter.notifyDataSetChanged()

Comment on lines +53 to +55
/**
* 좌우 스와이프
*/

Choose a reason for hiding this comment

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

이렇게 여러 줄 주석처리하는 단축키가 뭔가요? 저는 맨날 //만 써서 ... 너무 궁금하네요.

Choose a reason for hiding this comment

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

제발요 ...

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅋㅋㅋ급해보이셔서 남겨봅니다...
cmd+shift+/ 로 구간 드래그 후 해보시길 바랍니당. 빠른 시일내에 다른 코드리뷰 답글 달아보겠습니다 감사드립니다 :)

/**
* 좌우 스와이프
*/
override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {}

Choose a reason for hiding this comment

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

FollowerAdapter에서 onItemSwipe 메서드가 있던데, 이걸 활용해서 onSwiped() 메서드 내부를 채우면 되지 않을까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

오호라호라!!!! 반영하겠습니다아!!

Copy link

@kkk5474096 kkk5474096 left a comment

Choose a reason for hiding this comment

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

자책할 실력이 아니십니다.
엄청난 성장을 하신듯합니다
한 수 배우겠습니다

@papajj06 papajj06 merged commit 8922a55 into main May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

seminar2
4 participants