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

お気に入り画面の新着タブのRecyclerViewとCoordinatorLayout実装 #20

Merged
merged 26 commits into from Oct 16, 2021

Conversation

nemo-855
Copy link
Collaborator

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

概要

・お気に入り画面の新着タブのrecyclerView実装
・CoordinatorLayout上側のレイアウトにのみ適用

関連情報

スクリーンショット

Before After

懸念点

お気に入りの文字の大きさが少し変わるところ
2段階?のCoordinatorLayoutとRefreshLayoutが合わさっているところどうすれば良いかわからない

@Naoki-Hidaka
Copy link
Collaborator

CoordinatorLayoutはgifがあった方がわかりやすいかも!
確か最近はmp4も対応してるからmp4でもOK

@nemo-855
Copy link
Collaborator Author

nemo-855 commented Oct 11, 2021

CoordinatorLayoutはgifがあった方がわかりやすいかも! 確か最近はmp4も対応してるからmp4でもOK

お忙しい中ありがとうございます🙇‍♂️本当時間がある時で構わないので、レビューお願いします。動画mp4で上げました!クリックすると見えると思います!

@Naoki-Hidaka
Copy link
Collaborator

本家の「お気に入り」の文字の大きさが変わるのは難しそうでした?

@nemo-855
Copy link
Collaborator Author

本家の「お気に入り」の文字の大きさが変わるのは難しそうでした?

パッとわからなかったので一旦そこを飛ばしました…。調べてみます!

@nemo-855
Copy link
Collaborator Author

FavoriteNewItemFragmentで無駄にbindingをFragmentのメンバ関数にしていたのでそこも修正しました!


@HiltViewModel
class FavoriteNewItemViewModel @Inject constructor() : ViewModel() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

せっかくなのでAPIから返ってきた想定でデータはこっちで持っておきましょうか

Copy link
Collaborator

Choose a reason for hiding this comment

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

あと多分お気に入りのブランドがあるかないかで表示が結構変わるのでそこも実装してもらえると 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

APIからでーた持ってきた前提に修正しました!!

お気に入りのブランドがあるところはまた長くなりそうなので別のプルリクであげます!

FavoriteNowPopular()
)

private fun createItemCells() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ちょっと書き方がいけてないので戻り値をListにして、updateListの方でupdate(defaultViewList + createItemCells())とかしたいですね

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

apiからデータ持ってきた前提でViewModelの方にループの処理は移しました!

app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintDimensionRatio="h,5:1">

<androidx.constraintlayout.widget.Guideline
Copy link
Collaborator

Choose a reason for hiding this comment

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

MaterialDesign的には左右16dpのmarginなんですが、16dpだと大きかったですか?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

純粋に見逃してました!🙏

コンポーネントに直接marginつけるのよくないかもと思い直したので、FavoriteItemCell内部でbindのタイミングで動的に左右に16dpのmarginをセットするようにしました!

Copy link
Collaborator

Choose a reason for hiding this comment

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

コンポーネントに直接marginつけるのよくないかもと思い直したので

お?これはどんな理由があります?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

純粋に同じレイアウト再利用するときに画面によってマージンが変わるかもしれないからです!ただ今回はそこまで入らない気がしますが笑

Copy link
Collaborator

Choose a reason for hiding this comment

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

基本的には左右16dpのマージンは変わらないのと、もしマージンが違うデザインだった場合、もはやそれは違うitemなんじゃないかなとは思いますね

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

確かにそれもそうですね!修正しときます!ありがとうございます🙏

android:id="@+id/arrow_image"
android:layout_width="0dp"
android:layout_height="0dp"
app:layout_constraintStart_toStartOf="@id/guideline_vertical2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

startなしで、endの制約をguideline_vertical2にすればこのImageViewのmarginはいらないのでは?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

コンポーネントに直接marginつけるのよくないかもと思い直したので、FavoriteItemCell内部でbindのタイミングで動的に左右に16dpのmarginをセットするようにしました!

android:layout_height="wrap_content"
<com.google.android.material.appbar.AppBarLayout
android:layout_width="match_parent"
android:layout_height="170dp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここはwrap_contentじゃダメでした?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

勘違いをしていたんですが、CollapsingToolbarLayoutがある程度の高さを持っていればレイアウトは崩れないのでTabLayoutとAppBarLayoutはwrap_contentで良いんですね!!高さのところ一番詰まったので勉強になりました!!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

今更ですが
確かAppbarLayoutは子ViewのminHeightまで小さくなります
CollapsingToolbarLayoutは何も指定しないとminHeightが?attr/actionBarSizeになっています
なのでCollapsingToolbarLayoutだけ消えてToolbarが残るという形になっています

<com.google.android.material.tabs.TabLayout
android:id="@+id/favorite_tab_layout"
android:layout_width="match_parent"
android:layout_height="50dp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

上と同様に修正しました!

android:id="@+id/favorite_view_pager"
android:layout_width="match_parent"
android:layout_height="match_parent"
app:layout_behavior="@string/appbar_scrolling_view_behavior" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここのlayout_behaviorはいらないような

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

修正しました!

app/src/main/res/layout/fragment_favorite_new_item.xml Outdated Show resolved Hide resolved
@Naoki-Hidaka
Copy link
Collaborator

追加でコメントしちゃいました

@nemo-855
Copy link
Collaborator Author

次のプルリクでお気に入りとかタップする挙動出てくると思うんで少しずつDataBinding導入します!

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.

長くなりましたが、対応ありがとうございました
マージンの修正だけコミットしたらマージしちゃいますね

@nemo-855
Copy link
Collaborator Author

nemo-855 commented Oct 16, 2021

修正しました!!こちらこそ細かいところまでレビューいただきめっちゃ勉強になりました!!

@Naoki-Hidaka Naoki-Hidaka merged commit 8a30ba3 into main Oct 16, 2021
@Naoki-Hidaka Naoki-Hidaka deleted the apply_coordinator_layout_favorite_fragment branch October 16, 2021 14:12
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

2 participants