最新の日報の表示 #147

Open
wants to merge 9 commits into
from

Projects

None yet

3 participants

@HISAS
Contributor
HISAS commented Dec 14, 2016 edited

インターンの新しいデザインで、右カラムに最大10件最新の日報を表示するようにしました。

@komagata komagata temporarily deployed to interns-com-pr-147 Dec 14, 2016 Inactive
@HISAS HISAS added the in progress label Dec 14, 2016
@HISAS HISAS changed the title from improve recent reports to 最新の日報の表示 Dec 14, 2016
@HISAS HISAS requested a review from komagata Dec 14, 2016
@HISAS
Contributor
HISAS commented Dec 14, 2016

@komagata レビューお願いします。

@komagata komagata temporarily deployed to interns-com-pr-147 Dec 14, 2016 Inactive
@komagata
Member

@HISAS circleciがコケているので確認して下さい。

@komagata
Member

@HISAS circleciがコケているので確認して下さい。

レビュー依頼は上記が全部OKだったことを確認してから提出してください。

@HISAS
Contributor
HISAS commented Dec 16, 2016

@komagata すみません。テストを通したのでご確認お願いします。

@machida machida added the review label Dec 16, 2016
@@ -2,6 +2,11 @@ class ApplicationController < ActionController::Base
protect_from_forgery
before_action :init_user
before_action :allow_cross_domain_access
+ before_action :recent_reports
+
+ def recent_reports
@komagata
komagata Dec 16, 2016 Member

これだとPOSTDELETE時も必ずこの処理が走るので、helperにrecent_reportsを作るのがいいと思います。

app/views/layouts/application.html.slim
@@ -26,4 +26,9 @@ html.is-application lang="ja"
main.page
= yield
- unless yield(:page_classes).include?('no-recent-reports')
- = render 'reports/recent_reports'
+ - if @reports.nil? == false
@komagata
komagata Dec 16, 2016 Member
- if @reports.present?

が良いと思います。

ifの条件でbooleanと比較するのは意味ないです。

app/views/layouts/application.html.slim
+ .recent-reports
+ .recent-reports__items
+ - @reports.each do |report|
+ - if report.present?
@komagata
komagata Dec 16, 2016 Member

reportが存在しない場合ってありますか?

@HISAS
Contributor
HISAS commented Jan 11, 2017

@komagata 遅くなってしまい申し訳ありません。修正しましたのでレビューお願いします。

app/helpers/reports_helper.rb
@@ -0,0 +1,5 @@
+module ReportsHelper
+ def recent_reports
+ @reports = Report.limit(10).order(updated_at: :desc, id: :desc)
@komagata
komagata Jan 11, 2017 Member

そんなことはないと思うんだけど、limit, orderの順番だと、id順の10件をとってそれをsortしてるように見えてしまうから、メソッドを呼び出す順番を変えた方が良いかも。

@komagata

recent_reportsの修正お願いします。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment