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

日報の検索機能を追加 #130

Merged
merged 9 commits into from Dec 6, 2016
Merged

日報の検索機能を追加 #130

merged 9 commits into from Dec 6, 2016

Conversation

hattori6789
Copy link
Contributor

@hattori6789 hattori6789 commented Nov 25, 2016

connected #123

Todo

  • 検索フォームをヘッダーに設置
    • プレイスホルダーに日報検索と表示
  • 検索ワードが存在する時、検索ワードの存在するレコードのみ取得
  • 検索ワードを使った日報の本文を表示
    • 表示される検索ワードをハイライト
    • 取得したレコードを更新日を基準にして直近の日報から表示
  • 検索結果画面にて<検索ワード> の検索結果と表示
  • 検索フォームのデザインをどのようにするべきか@machidaさんに伺う
  • 検索範囲に日報のタイトルを含める
    • 表示される検索ワードをハイライト

@komagata komagata temporarily deployed to interns-com-pr-130 November 25, 2016 15:38 Inactive
@hattori6789
Copy link
Contributor Author

@komagata @machida 日報の検索機能、現在の進捗[WIP]をアップします。
以下、参考画像です。Todoと合わせて参考にして頂けると幸いです。
2016-11-28 10 24 23

@@ -68,7 +68,12 @@ def report_params
end

def set_reports
@reports = Report.order(updated_at: :desc, id: :desc)
if params.key?(:word)
Copy link
Member

Choose a reason for hiding this comment

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

set_reports@search_wordも代入してるとわかりにくいので別の名前にするか処理の分け方を別にしたほうがいいと思います。

@@ -67,8 +68,16 @@ def report_params
)
end

def set_search_word
@search_word = params[:word]
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

前回頂いたレビューから時間が経ってしまい、申し訳ありません。レビューの中で、処理の分け方を別にした方が良いといったお話がありましたが、上記の修正で合っていますか?
それとも、app/models/search.rbファイルを作成し、model内にLike検索の処理を記述し、app/controllers/reports_controller.rbで処理を呼び出す方が良いということでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

上記であってます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます!

@@ -67,8 +68,16 @@ def report_params
)
end

def set_search_word
@search_word = params[:word]
end
Copy link
Member

Choose a reason for hiding this comment

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

上記であってます。

@hattori6789
Copy link
Contributor Author

hattori6789 commented Dec 5, 2016

@machida 検索フォームのデザインについて、どのようにしたら良いかアドバイス頂きたいです。
検索機能の実装を開始した初めの頃、一応自分でも以下のようなイメージを考えてみました。
interns-search-form

@hattori6789
Copy link
Contributor Author

検索範囲にタイトルを含めました。

参考画像

2016-12-05 13 14 06

@hattori6789 hattori6789 changed the title [WIP]日報の検索機能を追加 日報の検索機能を追加 Dec 5, 2016
@hattori6789
Copy link
Contributor Author

@komagata レビューお願いします!
デザインに関しては、@machidaさんが対応して下さることなりました。詳細は、本日のSlackの、internsプロジェクトのタイムラインを参照して頂きたいです。https://fjord.slack.com/archives/intern/p1480904439000003

@machida
Copy link
Member

machida commented Dec 5, 2016

image

とりあえずのデザイン入れておきました

@machida
Copy link
Member

machida commented Dec 5, 2016

@hattori6789 コンフリクトしてたので master をrebase しました。このブランチをいじるときは、ローカルのを一旦消してから、このブランチを取り込んで作業お願いします。

@hattori6789
Copy link
Contributor Author

hattori6789 commented Dec 5, 2016

@machida ローカルのを一旦消してからというのは、ローカルのsearch_pageを消すということで合ってますか?それともmasterの方でしょうか?

@machida
Copy link
Member

machida commented Dec 5, 2016

@hattori6789 はい、commit の順番を変えたので、これをpull するとエラーが出ると思います。

@machida
Copy link
Member

machida commented Dec 5, 2016

search_page のほうですー

@hattori6789
Copy link
Contributor Author

@machida かしこまりました!ありがとうございます!

@hattori6789
Copy link
Contributor Author

@machida デザインしていただいたものを手元で動かしてみました。デザインで綺麗にしてもらったものが動いているのを見ると、より嬉しい😆

@machida
Copy link
Member

machida commented Dec 5, 2016

@hattori6789 #129 こっちのブランチにも取り込みましたー

@hattori6789
Copy link
Contributor Author

@machida ありがとうございます!新デザインでも触ってみてます。
新しいページが!!!👀

@hattori6789
Copy link
Contributor Author

@komagata @machida 今回のように、実装した機能を別のPRに取り込んでいただいた場合、こちらのPRはクローズして大丈夫でしょうか?もしレビューがオーケーだったら、ちょっと今の本番環境で検索機能触ってみたい気もしてます😌

@machida
Copy link
Member

machida commented Dec 5, 2016

@hattori6789 今回は新デザインの前にmasterに取り込んでリリースしたいので、masterに取り込まれてからcloseでお願いしますー

@hattori6789
Copy link
Contributor Author

@machida かしこまりました!

@komagata
Copy link
Member

komagata commented Dec 6, 2016

@hattori6789

LGTM

@komagata komagata merged commit 5c28e05 into master Dec 6, 2016
@komagata komagata deleted the search_page branch December 6, 2016 05:59
@hattori6789
Copy link
Contributor Author

@komagata ありがとうございます😂

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