トップページに一覧表示（サーバーサイド）

dddrvshhh commented 4 years ago

What

一覧表示のサーバーサイドを実装した

Why

トップに表示させるページだから

TOMARUMARU commented 4 years ago

conflict解消時にレビューした箇所が損なわれてしまう可能性もありますので、レビュー依頼の前にconflictの解消をお願い致します。 ↓

dddrvshhh commented 4 years ago

分かりました。コンフリクト解消後にまたお願いします。

dddrvshhh commented 4 years ago

画像はまだ登録されていないので仮です。priceとnameだけ一覧表示に表示しました。コンフリクトは解消済みです。他にも、他で実装されていないのでコードで書くとエラーが出る部分（.where.not(condition: 1).where(condition: 0)）等はコメントアウトしています。レビューよろしくお願いします

dddrvshhh commented 4 years ago

コメントアウトに対して、情報共有のためにコメントアウトの理由や目的をコード中にコメントしました。ビューの、「スタイルを定義していないので、不要でしたら削除しましょう。」についてですが、とりあえずサーバーサイドを見てもらおうと思い、スタイルの定義を後回しにして提出しました。そのため、このあとすぐに使う予定なのですが、それでも一旦消去した方が良いでしょうか？

dddrvshhh commented 4 years ago

同時ビューとサーバーサイドを同時並行で作っていたので、ブランチが同じになってしまいました。分かりにくくて申し訳ありません。【マークアップ】トップページの実装が完了いたしましたのでレビューお願いします

miyagit commented 4 years ago

同時ビューとサーバーサイドを同時並行で作っていたので、ブランチが同じになってしまいました。分かりにくくて申し訳あり> ません。【マークアップ】トップページの実装が完了いたしましたのでレビューお願いします

こちら別のブランチ名にした方が良いですね。git branch -mですぐブランチ名を変えれるので、ブランチ名を変えて新たにPRを作りましょう。

こちらのPRに関しては、ビューを作ってる人がサーバーサイドを取り込む前のコミットを持っていると思うのでforce pushしてサーバーサイドの差分を削除しましょう。 git push -fとつけるとforceになります。

dddrvshhh commented 4 years ago

git push forceを試みたのですが、Git の push --force は共有レポジトリにプッシュされた他の変更を破壊する可能性がある等で自分では理解がまだ及ばず、実行できませんでした。また、マークアップ（【マークアップ】トップページ）とサーバーサイド（【サーバサイド】商品一覧表示）を同時に仕上げるくらいのつもりで作っていたので、サーバーサイドとマークアップのコミットはかなり細かくなってしまっていて、分けるのは少し難しいです。新たにブランチを切る場合、今のビューの記述をコピーして避難させてから削除し、もう一度コピペする、という風になってしまうのですがよろしいでしょうか？それか、なんとかこちらのプルリクエストで見てもらうことは出来ないでしょうか？

i-date commented 4 years ago

コメント返信ありがとうございます！本当は細かくプルリクを作成するのが望ましいのですが、今回既にマージされてしまっているので、こちらのプルリクで現時点の差分を確認いたしました。拝見したところ大きな問題点はないようですのでLGTMとさせていただきます。次回からは1プルリク1機能（画面）のようになるように意識してくださると、よりベターなプルリクとなると思います。

下記はご参考までに。細かいプルリクや大きなプルリクについての参考を下記に載せますので、よろしければどういうことか一例としてご確認ください。 https://khigashigashi.hatenablog.com/entry/2018/03/09/020359

naomaru-t / fleamarket_sample_80d