模擬案件_#2_addFeedPage

yasyun commented 3 years ago

・Issue

#2

・レビューして欲しい点

FeedPageViewController.swiftとFeedPageCell.swiftを中心に見ていただきたいです。

・その他

DataModel.swiftはFeedPageで使っていないstructの中身を削除して、LGTM数を取得するために要素を追加したため、差分が出ています。
TagModel.swiftは一度間違えて削除してしまい作り直したので差分が出ていますが、中身は何も変更していないです。
ArticleModel.swiftは不要になったファイルなのでXcode上で削除したのですが、なぜか今回一緒にpushされてしまったため、無視していただいて大丈夫です。後ほど修正します。　⇒　削除しました。
前回のレビューの際に指摘をいただき修正したところには、コメントを付けてあります。
エラー画面の実装に関しては、最後にまとめてエラー画面用のissueにて修正しようと思っています。

mcz9mm commented 3 years ago

ブランチ名がちょっと雑かな。 developやfeatureを手前につけて、issue番号を振ってあげると管理しやすいと思う。 https://qiita.com/c6tower/items/fe2aa4ecb78bef69928f

develop/#2 feature/#2

ShuheiYoshidaJP commented 3 years ago

その2 コメントしました。 SearchBarとTableViewの間に空いているスペースが気になります。イメージ

yasyun commented 3 years ago

その2 コメントしました。 SearchBarとTableViewの間に空いているスペースが気になります。

スペースを埋めるように修正しました。

yasyun commented 3 years ago

ページネーションの仕組みについては変更した方が良いのでしょうか？

yasyun commented 3 years ago

ブランチ名がちょっと雑かな。 developやfeatureを手前につけて、issue番号を振ってあげると管理しやすいと思う。 https://qiita.com/c6tower/items/fe2aa4ecb78bef69928f

develop/#2 feature/#2

ローカルのブランチ名は変更したのですが、リモートのブランチ名はひとまず次回以降から準拠させる形で大丈夫でしょうか？

mcz9mm commented 3 years ago

AutoLayoutの制約が壊れてるログが出てるね

2021-07-09 10:22:11.551571+0900 Qiita_App[98897:2480547] [LayoutConstraints] Unable to simultaneously satisfy constraints.
    Probably at least one of the constraints in the following list is one you don't want. 
    Try this: 
        (1) look at each constraint and try to figure out which you don't expect; 
        (2) find the code that added the unwanted constraint or constraints and fix it. 
(
    "<NSLayoutConstraint:0x6000000a7610 Qiita_App.EnhancedCircleImageView:0x7fa2af608b30.height == 50   (active)>",
    "<NSLayoutConstraint:0x6000000a7a20 UITableViewCellContentView:0x7fa2af618370.bottomMargin == Qiita_App.EnhancedCircleImageView:0x7fa2af608b30.bottom + 6   (active)>",
    "<NSLayoutConstraint:0x6000000a7a70 Qiita_App.EnhancedCircleImageView:0x7fa2af608b30.top == UITableViewCellContentView:0x7fa2af618370.topMargin   (active)>",
    "<NSLayoutConstraint:0x6000000a7de0 'UIView-bottomMargin-guide-constraint' V:[UILayoutGuide:0x600001a890a0'UIViewLayoutMarginsGuide']-(11)-|   (active, names: '|':UITableViewCellContentView:0x7fa2af618370 )>",
    "<NSLayoutConstraint:0x6000000e6670 'UIView-Encapsulated-Layout-Height' UITableViewCellContentView:0x7fa2af618370.height == 78.3333   (active)>",
    "<NSLayoutConstraint:0x6000000a7d40 'UIView-topMargin-guide-constraint' V:|-(11)-[UILayoutGuide:0x600001a890a0'UIViewLayoutMarginsGuide']   (active, names: '|':UITableViewCellContentView:0x7fa2af618370 )>"
)

mcz9mm commented 3 years ago

次からマストでお願いします

ローカルのブランチ名は変更したのですが、リモートのブランチ名はひとまず次回以降から準拠させる形で大丈夫でしょうか？

yasyun commented 3 years ago

AutoLayoutの制約が壊れてるログが出てるね

2021-07-09 10:22:11.551571+0900 Qiita_App[98897:2480547] [LayoutConstraints] Unable to simultaneously satisfy constraints.
  Probably at least one of the constraints in the following list is one you don't want. 
  Try this: 
      (1) look at each constraint and try to figure out which you don't expect; 
      (2) find the code that added the unwanted constraint or constraints and fix it. 
(
    "<NSLayoutConstraint:0x6000000a7610 Qiita_App.EnhancedCircleImageView:0x7fa2af608b30.height == 50   (active)>",
    "<NSLayoutConstraint:0x6000000a7a20 UITableViewCellContentView:0x7fa2af618370.bottomMargin == Qiita_App.EnhancedCircleImageView:0x7fa2af608b30.bottom + 6   (active)>",
    "<NSLayoutConstraint:0x6000000a7a70 Qiita_App.EnhancedCircleImageView:0x7fa2af608b30.top == UITableViewCellContentView:0x7fa2af618370.topMargin   (active)>",
    "<NSLayoutConstraint:0x6000000a7de0 'UIView-bottomMargin-guide-constraint' V:[UILayoutGuide:0x600001a890a0'UIViewLayoutMarginsGuide']-(11)-|   (active, names: '|':UITableViewCellContentView:0x7fa2af618370 )>",
    "<NSLayoutConstraint:0x6000000e6670 'UIView-Encapsulated-Layout-Height' UITableViewCellContentView:0x7fa2af618370.height == 78.3333   (active)>",
    "<NSLayoutConstraint:0x6000000a7d40 'UIView-topMargin-guide-constraint' V:|-(11)-[UILayoutGuide:0x600001a890a0'UIViewLayoutMarginsGuide']   (active, names: '|':UITableViewCellContentView:0x7fa2af618370 )>"
)

修正しました。

yasyun commented 3 years ago

今の状態だとかなりスクロールがカクついてるので改善しないといけないかもね。

ページネーションの仕組みについては変更した方が良いのでしょうか？

画像のキャッシュ化をしたところ、ページネーションの仕組みは変えていないのですが、かなりスムーズに動くようになりました。しばらく上下に動かしてましたが、ガクガクする事はなくなりました。一応これで改善したように思えるのですが、ページネーションの仕組みも変更した方がよいでしょうか？

yasyun commented 3 years ago

なぜか同じcommitが２回されていることになってしまいました。すみません。

mcz9mm commented 3 years ago

定例でも伝えたけど画像読み込みでメインスレッドを遮っているせいでカクつきが発生してたと思うよ。ページネーションは一旦このままで良いです。

画像のキャッシュ化をしたところ、ページネーションの仕組みは変えていないのですが、かなりスムーズに動くようになりました。しばらく上下に動かしてましたが、ガクガクする事はなくなりました。一応これで改善したように思えるのですが、ページネーションの仕組みも変更した方がよいでしょうか？

mcz9mm commented 3 years ago

LGTM

shinonome-inc / mobile_sakai

模擬案件_#2_addFeedPage #19