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

お気に入り画面のアイテムタブの作成 一個前のプルリクはクローズしました #29

Merged
merged 52 commits into from Oct 24, 2021

Conversation

nemo-855
Copy link
Collaborator

@nemo-855 nemo-855 commented Oct 17, 2021

概要

お気に入りのアイテムタブを実装

関連情報

関連するPR,issueがあればここにリンクを貼る

スクリーンショット

見た目の変更があるならそれを貼る

Before After

懸念点

またもgit操作でミスったのでプルリク作り直しました!!🙇‍♂️

まだ修正終わっていない点

① FavoriteItemFragmentのそれぞれのItemViewModelに対する isSameAs hasSameContentの定義
自身がないので考えをコメントで残しておきました!ご確認お願いします!!

@@ -16,4 +17,13 @@ class FavoriteItemNoItem : BindableItem<FavoriteItemNoItemRegisteredBinding>(),
override fun initializeViewBinding(view: View): FavoriteItemNoItemRegisteredBinding {
return FavoriteItemNoItemRegisteredBinding.bind(view)
}

/**現状FavoriteItemNoItemは一つしか表示しないので classが等しければ更新は不要*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

残したコメントこちらです!

@nemo-855 nemo-855 changed the title Create favorite item お気に入り画面のアイテムタブの作成 Oct 17, 2021
@nemo-855 nemo-855 changed the title お気に入り画面のアイテムタブの作成 お気に入り画面のアイテムタブの作成 一個前のプルリクはクローズしました Oct 17, 2021
}

private class CustomGroupieAdapter : GroupieAdapter() {
private var _itemList: List<BindableItem<out ViewBinding>> = makeDefaultList()
Copy link
Collaborator

Choose a reason for hiding this comment

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

これはなくせそう

private val adapter: CustomGroupieAdapter
) : GridLayoutManager.SpanSizeLookup() {
override fun getSpanSize(position: Int) =
(adapter.getItem(position) as SpanSizeInterface).spanSize
Copy link
Collaborator

Choose a reason for hiding this comment

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

SpanSizeInterfaceを継承してるものしか入ってこないのは明確ですが念のため
(adapter.getItem(position) as? SpanSizeInterface)?.spanSize ?: 0
みたいな感じでnull安全にcastして欲しいです

return FavoriteItemDescriptionBinding.bind(view)
}

override fun isSameAs(other: Item<*>): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(other as? FavoriteItemDescription)?.model == model
かな

return other.model.id == this.model.id
}

override fun hasSameContentAs(other: Item<*>): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

こっちが同じ要素を持ってるかどうか

return other is FavoriteItemNoItem
}

override fun hasSameContentAs(other: Item<*>): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

こっちのロジックはisSameAsと一緒なので
= isSameAs(other)
でよさそう

return other is FavoriteItemNowPopular
}

override fun hasSameContentAs(other: Item<*>): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@@ -0,0 +1,5 @@
package com.nemo.androiduitraining.view.fragment.favorite.entity.item

interface SpanSizeInterface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

interfaceにinterfaceと名前をつけることはあまりないので今回は
HasSpanSizeItem
とかでいいんじゃないかなと

@nemo-855
Copy link
Collaborator Author

nemo-855 commented Oct 19, 2021

hasSameContentAs周りに関してなんですけど、
modelがdata class の場合

 other.model == this.model

みたいな時 == が equalsと同じだから、要素が同じかどうかの比較だと思ってたんですけどここってどうなんですかね?

@Naoki-Hidaka
Copy link
Collaborator

modelが持ってる要素が同じかどうかを比べたいですね

@nemo-855
Copy link
Collaborator Author

nemo-855 commented Oct 20, 2021

model自体がdata classなんですけど、other.model == this.modelだとmodelの比較にならないですかね?

@Naoki-Hidaka
Copy link
Collaborator

model同士の比較はisSameAsの方ですかね
hasSameContentはidとかの比較だと思います

@nemo-855
Copy link
Collaborator Author

なるほど!
ずっと逆になってたんですね!!今気づきました!ありがとうございます!修正します🙇‍♀

Copy link
Collaborator

@Naoki-Hidaka Naoki-Hidaka left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Naoki-Hidaka Naoki-Hidaka merged commit ce31813 into main Oct 24, 2021
@Naoki-Hidaka Naoki-Hidaka deleted the create_favorite_item branch October 24, 2021 12:18
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.

None yet

3 participants