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.

Sign up for GitHub

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

Jump to bottom

特別イベント一覧、定期イベント一覧に、直近(近日開催)のイベント一覧を表示する。 #7272

Merged
merged 77 commits into from
Jul 2, 2024
Merged

特別イベント一覧、定期イベント一覧に、直近(近日開催)のイベント一覧を表示する。 #7272

merged 77 commits into from
Jul 2, 2024

Conversation

masyuko0222
Copy link
Contributor

@masyuko0222 masyuko0222 commented Jan 28, 2024 •
edited
Loading

Issue

参考にしたIssue(PR)

概要

特別イベント一覧(/events)、定期イベント一覧(/regular_events・/regular_events?target=not_finished)にも、「近日開催のイベント」一覧を表示させました。

ダッシュボード(/)に表示されている「近日開催のイベント」一覧を参考にしていますが、デザインは異なるものを実装します。

ダッシュボード(/)の例

無題

変更確認方法

  1. feature/list_upcoming_events_on_special_and_regular_events_pagesをローカルに取り込む
  2. bin/rails db:resetでDBをクリーンアップする
  3. foreman start -f Procfile.devで起動する
  4. ユーザーkimuraでログインをする

  1. 以下の各ページの「近日開催のイベント」コンポーネントに、手順6.のイベント群が表示されていることを確認する
  • 特別イベント一覧(/events)
  • 全ての定期イベント一覧(/regular_events)
  • 開催中の定期イベント一覧(/regular_events?target=not_finished)

  • 今日開催のイベント
    • ダッシュボード表示確認用テストイベント(当日用)
    • kimura専用イベント
    • ダッシュボード表示確認用テスト定期イベント
  • 明日開催のイベント
    • ダッシュボード表示確認用テストイベント(翌日用)
    • 就職関係かつ直近イベントの表示テスト用
    • ダッシュボード表示確認用テスト定期イベント
  • 明後日開催のイベント
    • ダッシュボード表示確認用テストイベント(明後日用)

※上記以外のイベントが表示されていても問題ありません。

Screenshot

※新しい機能のため、変更後のScreenshotsのみを貼ります。
※/events・/regular_events・/regular_events?target=not_finishedの画面変更点は同じです。
※下のスクリーンショットは/eventsページのものとなります。
無題

@masyuko0222 masyuko0222 force-pushed the feature/list_upcoming_events_on_special_and_regular_events_pages branch from 976c6a0 to 1c58590 Compare January 29, 2024 10:59
@masyuko0222 masyuko0222 force-pushed the feature/list_upcoming_events_on_special_and_regular_events_pages branch 2 times, most recently from 6d34959 to cc6410e Compare February 4, 2024 14:13
@machida

This comment was marked as resolved.

Sign in to view
@masyuko0222

This comment was marked as resolved.

Sign in to view
@masyuko0222 masyuko0222 marked this pull request as ready for review February 13, 2024 11:50
@masyuko0222 masyuko0222 requested review from junohm410 and removed request for junohm410 February 13, 2024 11:52
@masyuko0222 masyuko0222 force-pushed the feature/list_upcoming_events_on_special_and_regular_events_pages branch from bdb3241 to 454c050 Compare February 13, 2024 12:43
@masyuko0222
Copy link
Contributor Author

masyuko0222 commented Feb 13, 2024 •
edited
Loading

@machida
度々すみません。
ブラウザサイズによって、イベントが無いときの「近日開催イベント」表示の仕方が違うかもしれません。こちらお間違えなかったでしょうか👀

以下は、Event・RegularEvent共にdestroy_allをした後の画面になります。
(環境依存でしたらすみません)

横1366 縦728

image

横758 縦700

image

イベントがない場合は、後者のように「◯◯開催のイベントはありません」とウィンドウサイズに限らず、表示したいと思っていました。

お手数ですがご確認よろしくお願いいたします。

@machida
Copy link
Member

machida commented Feb 13, 2024

@masyuko0222
お、メインカラムが空の場合を想定してませんでした。サイドカラムはメインカラムの高さに依存があるので、メインカラムが空の場合に、サイドカラムが下まで伸びないというバグがありました。
メインカラムの高さに min-height を設定し、もし空であってもある程度高さを保ち、サイドカラムが下まで伸びるようにしておきました。

@masyuko0222 masyuko0222 marked this pull request as draft February 14, 2024 02:32
@masyuko0222 masyuko0222 force-pushed the feature/list_upcoming_events_on_special_and_regular_events_pages branch from 6d277cf to 9b25ab6 Compare February 14, 2024 10:12
@masyuko0222 masyuko0222 marked this pull request as ready for review February 14, 2024 12:18
@masyuko0222 masyuko0222 removed the request for review from junohm410 February 14, 2024 12:50
@masyuko0222
Copy link
Contributor Author

masyuko0222 commented Feb 14, 2024 •
edited
Loading

@machida
お忙しいところ失礼します。

/regular_eventsページだと表示が下図のようになってしまいますので、お時間ある際に修正していただけますと幸いです。

無題

/eventsページは問題ありません。

@masyuko0222 masyuko0222 marked this pull request as draft February 14, 2024 12:55
@machida
Copy link
Member

machida commented Feb 14, 2024

@masyuko0222 デザイン崩れを修正しましたー

@masyuko0222
Copy link
Contributor Author

@machida
確認がとれましたー！お忙しいところ何度もありがとうございました！

@junohm410
ご都合よろしければこちらのレビューをお願いしたいです。
忙しかったら、遠慮なく仰ってくださいー🙇

@masyuko0222 masyuko0222 marked this pull request as ready for review February 14, 2024 14:05
@junohm410
Copy link
Contributor

@masyuko0222
お疲れ様です。レビュー承知いたしました👍
遅くて土日くらいまでにお戻しできるように努めます🙏
よろしくお願いいたします。

junohm410

This comment was marked as resolved.

Sign in to view

@masyuko0222 masyuko0222 force-pushed the feature/list_upcoming_events_on_special_and_regular_events_pages branch from 8792fbc to 47c4178 Compare February 17, 2024 05:05
@masyuko0222
Copy link
Contributor Author

@junohm410
レビューありがとうございました！再レビューよろしくお願いいたします！

  • コードは全部ご指摘通りの修正をしました。それぞれ返信しています。
  • 冒頭にあった変更確認手順についても修正しました。これは自分が勘違いしていました、お手数おかけしてすみません。

また、コミットメッセージについてもありがとうございます。
リファクタリングのコミットは、「rubocopの修正」レベルの粒度で考えちゃっていました・・・。
リファクタリングにも自分の意図や意志が入ってくるので、ちゃんとメッセージを書いてあげた方がいいですね。以降気を付けます。

@masyuko0222
メソッドの責務を分けた
6abbb0d
@masyuko0222
等価のオーバーライドのし忘れでテストが落ちていたので修正
fa2f5b8
@masyuko0222
UIに空白が出てしまう不具合を修正
754594c
@masyuko0222
日付の定義はメソッド内にした
735e1a2 
クラスの定数で定義してしまうと、取得タイミングで常に固定されてしまうため、メソッド内に移動
@masyuko0222
不要な変数を削除
07df338 
説明変数的に書いていたが、説明するほどでもなかった。
@masyuko0222
UpcomingEventのinitializeでscheduled_dateを受け取るように
15ee856 
Viewで使うとき使いやすいし、コンストラクタの引数にあっても違和感はないため
@masyuko0222
不要な引数を削除
6125fd1 
インスタンス自身が知っているため、引数がなくても内部で使えるため。
@masyuko0222
今後の予定日を集める責務を移動
1e04133 
RegularEvent自身は今後のイベント日を知っていても違和感はないので、わざわざ別クラスに責任を持たせる必要が無いと思ったため。
@masyuko0222
引数渡し忘れを修正
ac50e3f
@masyuko0222
TimeWithZoneクラスを利用するようにした。
8a009f9 
Dateクラスを利用すると検索時にUTCに変換してくれずバグになるため。
@masyuko0222
引数がデフォルト引数になってしまっていたので修正
a2a1ec2
@masyuko0222
より正確な変数名にした
f7e4508
@masyuko0222
クラスメソッドで定義していたgather_events_scheduled_onをscopeに移動した
43263cf
@masyuko0222
命名変更
f9b73ba 
「第〇週のX曜日」ではなく、「第〇X曜日」を表すので、それに沿った命名にした。
@masyuko0222
filterメソッドを使うようにした
9c395f9 
selectだと、SQLのselectが期待されてしまうため、勘違いさせないように。
@masyuko0222
不要な説明変数を削除
3e0b311
@masyuko0222
命名変更
cbd7790 
held?だと引数が必要そうなので、held_scheduled_date?だと内部で処理している感がでる。
@masyuko0222
無駄な改行や、テスト名直し忘れていたところを直した
8c6b642
@masyuko0222
UpcomingEvent内でGroupを作るように
10f934b 
UpcomingEventsGroupクラスを作成していたが、近日開催イベントであるUpcomingEvent自体がその作成を担っても違和感はないため。
@masyuko0222
英語的におかしいので、onを前置詞にいれた
eb745a4
@masyuko0222
戻り値をハッシュに変えた
1c5b79b 
Structだと内部でしか使わないことを想定されるが、ContorllerでViewに渡すためにStructを使ってしまってる感じだったので変更した。
クラスもあまり増やしたくなかったので、シンプルにハッシュに変えた。
あと利用していないkeyが潜んでいたのでそれも消した。
@masyuko0222
UpcomingEventsGroupクラスを作った
571a0e9 
・クラスとして定義することで、どのような構造かすぐにわかるようにした。
・また、副作用がないので、利用者からはUpcomingEventのメンバに見えるようにした。
  - 参考：https://docs.komagata.org/5630
@masyuko0222
クラスメソッドはclass<<selfで統一した
f8e3fef
@masyuko0222
.build_groupもパブリックメソッドにした。
5d649e6 
・UpcomingEventの責務として、外部に公開しておくべきメソッドのため。
・.upcoming_events_groupsの内部で利用されているため、テストはそちらで担保する。
  - テストコード見やすくするために、コード順も少し変えた。
@masyuko0222
テスト名を変更
bdc7b96 
・メソッド名のみのテスト名ではない場合は、自然な英文にするが慣習のため、一部テスト名を自然な英文にした。
・逆に１つのテストケースに合併しても問題ないものは合併して、メソッド名のみのテスト名にした。
・また、併せて説明変数に代入している処理行の位置を変更して、メインのテストがどこになるかもわかりやすくした。
@masyuko0222 masyuko0222 force-pushed the feature/list_upcoming_events_on_special_and_regular_events_pages branch from a20e780 to bdc7b96 Compare July 1, 2024 13:43
komagata
komagata approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

長いやりとり大変お疲れさまでした。
とってもコードが良くなったと思います！

@komagata komagata merged commit c56eeef into main Jul 2, 2024
2 checks passed
@komagata komagata deleted the feature/list_upcoming_events_on_special_and_regular_events_pages branch July 2, 2024 22:18
@github-actions github-actions bot mentioned this pull request Jul 2, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machida machida machida left review comments

@komagata komagata komagata approved these changes

@junohm410 junohm410 junohm410 approved these changes

Assignees

@masyuko0222 masyuko0222

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@masyuko0222 @machida @junohm410 @komagata