-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 바텀네비게이션에 서재탭 추가 #631
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
junseo511
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.
고생하셨습니다 👍 몇가지 리뷰만 확인해주세요
| entries.find { fragmentView -> | ||
| fragmentView.resId == id | ||
| } ?: throw IllegalArgumentException() | ||
| entries.find { it.resId == 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.
a: 이쪽이 가독성이 더 좋아진 것 같네요 👍
|
|
||
| private val libraryAdapter: LibraryViewPagerAdapter by lazy { | ||
| LibraryViewPagerAdapter( | ||
| novels = emptyList(), |
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.
c: 기본 생성자로 emptyList()를 두는 것과 초기화시 emptyList()를 넣어주는 것에는 어떤 차이점이 있을까요?
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.
emptyList()를 기본 생성자로 두는 것과 초기화시 두는 것은 동일한 동작을 하는 것으로 알고 있습니다! 하지만 설계적 관점에서는 차이가 있을 수 있는데 생성장에 기본값으로 두는 경우 해당 클래스가 내부적으로 어떤 초기 상태를 갖는지를 스스로 정의하는 것이기 때문에 빈리스트로 시작하다는 것을 명시적으로 전달할 수 있습니다! 반면 초기화시 두는 것은 생성하는 쪽에서 초기 상태를 명시적으로 결정할 수 있어서 책임의 위치가 보다 외부로 분리된다는 장점이 있다고 알고 있습니다! 이는 어댑터를 재사용할 때 더 유연하게 동작하게 만듭니다! 현재와 같이 외부에서 초기화를 관리하는 것이 어댑터를 보다 명시적으로 제어할 수 있는 장점이 있다고 생각합니다
| super.onViewCreated(view, savedInstanceState) | ||
| super.onViewCreated(view, savedInstanceState) |
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.
c: onViewCreated를 두 번이나 실행시킨 이유가 있으실까요?
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.
으앗 이건 정신없는 복붙에 향연 속 실수입니다...ㅎ
| private fun setupViewPagerAndTabLayout() { | ||
| setupViewPager() | ||
| setupTabLayoutWithViewPager() | ||
| setupObserver() |
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.
c: 함수명에 비해 많은 일을 하고 있는 것은 아닌지 생각해봅시다!
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.
으음 그러고 보니 그렇네염! 리뷰 감사합니다! 저는 setupObserver()만 따로 onViewCreated()만 분리해도 좋을 것 같은데 어떻게 생각하시나염
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.
으음 그러고 보니 그렇네염! 리뷰 감사합니다! 저는 setupObserver()만 따로 onViewCreated()만 분리해도 좋을 것 같은데 어떻게 생각하시나염
그런 방향도 괜찮게 보입니다 👍
|
|
||
| private fun setupTabLayoutWithViewPager() { | ||
| TabLayoutMediator(binding.tlLibrary, binding.vpLibrary) { tab, position -> | ||
| tab.text = StorageTab.fromPosition(position).title |
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.
c: fromPosition(position) 이라는 식이 조금 어색한데 fromPosition은 기본으로 제공되는 확장함수인가요?
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.
기본으로 제공되는 확장함수는 아닙니다! 탭 이넘 클래스에서 직접 정의한 정적 함수인데요! 처음 보시면 조금 어색하게 느껴지실 수 있다고 생각합니다 아마 저도 다른 분들의 코드를 참고하면서 사용했던 걸로 알고 이넘에서는 관용적으로 사용되는 함수명인 것 같아요!!
| binding.tlLibrary.addOnTabSelectedListener( | ||
| object : TabLayout.OnTabSelectedListener { | ||
| override fun onTabSelected(tab: TabLayout.Tab?) { | ||
| val selectedTab = requireNotNull(tab) { "Tab must not be null" } |
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.
c: require을 통해 강제하신 이유가 있으신가요?
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.
require로 둔 이유는 탭 파라미터가 null일 가능성이 없기 때문에 명시적으로 null이 될 수 없는 값이라는 것을 표시하고 싶었습니다!
| }.attach() | ||
|
|
||
| binding.tlLibrary.addOnTabSelectedListener( | ||
| object : TabLayout.OnTabSelectedListener { |
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.
a: 명시적인 리스너 활용 좋습니다 👍
| binding.clLibraryNull.visibility = | ||
| if (uiState.userNovelCount == 0L) View.VISIBLE else View.GONE | ||
|
|
||
| binding.vpLibrary.visibility = | ||
| if (uiState.userNovelCount > 0L) View.VISIBLE else View.GONE | ||
|
|
||
| binding.btnLibraryGoToSearchNovel.visibility = | ||
| if (uiState.userNovelCount == 0L) View.VISIBLE else View.GONE |
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.
c: 반복되는 상수는 const로 뺌은 어떠신지 궁금합니다~~
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.
앗 그러네염 반영하겠습니다~
|
|
||
| @SuppressLint("NotifyDataSetChanged") | ||
| fun updateNovels(newNovels: List<StorageNovelModel>) { | ||
| this.novels = newNovels |
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.
c: 여기서만 this를 명시해주신 이유가 있으실까요? 위에서는 그대로 사용하는 것으로 보이는데요!
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.
바로 위에서 newNovels라는 매개변수와 클래스의 novels 프로퍼티 간 이름이 유사해서 양쪽을 명확히 구분해 의도를 더 분명히 전달하기 위함이었는데 혹시 다른 의견있으신가여?
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.
저는 그냥 사용하는 다른 위치에서의 일관성을 생각했어요!! 본인이 좋으신 방향으로 고고하시죠~~
m6z1
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.
고생하셨어요 ~~ ! 👍
소소한 코멘트 확인 부탁드립니당
| setupViewPagerAndTabLayout() | ||
| setupInitialReadStatusTab() | ||
| onSortTypeButtonClick() | ||
| onExploreButton() |
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.
r: 요것도 postfix 에 Click 붙여주세요!
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.
네엡!!1
| } | ||
|
|
||
| private fun setupObserver() { | ||
| libraryViewModel.uiState.observe(requireActivity()) { uiState -> |
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.
r: 요거 requireActivity 말고 viewLifecycleOwner 사용해주세요!!
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.
네에엡!!
| LibraryItemAdapter(emptyList(), novelClickListener) | ||
| } | ||
|
|
||
| private val layoutManager: GridLayoutManager by lazy { |
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.
a: 이거 xml 에서 세팅해도 되지 않나욥 ??
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.
맞아욤! 지금은 고려하지 않아도 되는 사항인 것 같아서 xml에서 관리하도록 하겠습니담!
# Conflicts: # app/src/main/res/values/strings.xml
junseo511
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.
고생하셨슴당
📌𝘐𝘴𝘴𝘶𝘦𝘴
📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯
📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵
KakaoTalk_Video_2025-04-13-16-37-28.mp4
💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴
기존 보관함을 '나'와 '다른유저'가 공유하고 사용하면서 분기처리 되어졌던 부분은 삭제를 했습니다.
그 외에는 보관함은 건들이지 않았고 보관함이 기존에는 activity였기 때문에 fragment 형식으로 서재를 만들었습니다!
코드는 기존 보관함과 거의 유사하니 참고해주세요!!