ニコニコカレンダーをvue.jsにし、月ごとの表示にして、ページャーを付けた #2886

obregonia1 · 2021-06-26T05:44:31Z

概要

ニコニコカレンダーをvue.js化しました。
以前までは直近30日の日報を表示させていましたが、今回の変更でカレンダーを月ごとに表示し、ページャーを付けて月を移動できるようにしました。
なので、今回の変更でカレンダー上部にカレンダーの年月の表示とページャー< , >を追加し、カレンダーの各マスに表示していた月日表示05/12を日付のみの表示12に変更しています。

変更前

変更後

obregonia1 · 2021-06-26T06:21:30Z

@kasai441
こちらレビューよろしくお願いしますー🙏

kasai441 · 2021-06-28T05:35:02Z

@obregonia1
了解しましたー

obregonia1 · 2021-06-30T10:36:49Z

@kasai441
Lintを通し忘れていたので少し修正しています🙏

kasai441

@obregonia1

レビュー完了しました。カレンダーちゃんと動くのですごいですね！問題ないかと思いますー

細かい点ばかりコメントしています。Vue.jsもJSも不明な点が多く、間違った指摘となっているかもしれませんが、感想レベルでご覧ください。

コメント以外で気になった点

カレンダーをめくってリンクから日報に遷移した後、ブラウザバックすると最新の月に戻ってしまうのは、ユーザビリティが低そうです。が、少し調べた感じでは、これを直すのは難しそうなので、今後の拡張案として言及するだけに留めておきます。

kasai441 · 2021-06-28T07:47:05Z

app/decorators/user_decorator.rb

@@ -40,15 +40,6 @@ def url
    user_url(self)
  end

-  def niconico_calendar
-    reports_date_and_emotion = self.reports_date_and_emotion(CALENDAR_TERM)


CALENDAR_TERM は他に使用していないので、ファイル上部の宣言も削除していいと思いますー

見落としてました。ありがとうございます！

kasai441 · 2021-06-28T07:47:54Z

app/decorators/user_decorator.rb

-    last_wday = reports_date_and_emotion.first[:date].wday
-    blanks = Array.new(last_wday) { { report: nil, date: nil, emotion: nil } }
-
-    [*blanks, *reports_date_and_emotion].each_slice(DAYS_IN_WEEK)


同じくDAYS_IN_WEEKの宣言も

こちらも見落としてましたー。

kasai441 · 2021-06-29T07:37:38Z

app/javascript/niconico_calendar.vue

+      let week = []
+      let weekIndex = 1
+      let weekDay = 0
+      this.calendarSquares.forEach(function (date, i, ary) {


これも参考までですが、mapやforEach内の変数は hoges.forEach((hoge) => {})のように複数形と単数系が対応しているほうが読みやすいかもしれません。プロジェクト内のvueをgrepしましたがそういう書き方が多いようです。

確かにそうですね。変更しました。

kasai441 · 2021-06-30T06:33:14Z

app/javascript/niconico_calendar.vue

+      if (this.firstWday > 0) {
+        for (let blank = 0; blank < this.firstWday; blank++) {
+          calendar.push(null)
+        }


const calendar = Array(this.firstWday).fill(null)
Rubyではこんな感じの書き方をすると思いますが、JSではfor文を使った方がいいみたいな議論もあるようなので、参考まで。bootcampプロジェクト内でもArrayで初期化しているコードはvueでは無いみたいです。

なるほど、勉強になります:memo:

kasai441 · 2021-06-30T06:34:43Z

app/javascript/niconico_calendar.vue

+      for (let date = 1; date <= this.lastDate; date++) {
+        let result = null
+        if ((result = this.calendarReports.find(report =>
+            Number(report.reported_on.split('-')[2]) === date))) {


resultへの代入とif文を分けた方が読みやすいかもしれません

確かに読みにくいですね！resultの代入を分け、日報の日付を取得するメソッドを追加しました。

kasai441 · 2021-07-01T02:32:14Z

app/javascript/niconico_calendar.vue

+      }
+      return calendar
+    },
+    calendarWeeksAry() {


細かいですが、Aryという文字がなくてもcalendarWeeksで配列であることがわかるように思いました。project内には変数にAryをつけているものが見つからなかったのと、複数形で命名した変数に配列格納するパターンが多かったので揃えた方がいいかもしれません。

確かにそうですね〜。変更しました。

kasai441 · 2021-07-01T02:35:42Z

app/javascript/niconico_calendar.vue

+        week.push(date)
+        weekDay++
+        if (week.length === 7 || i === ary.length - 1) {
+          const weekObj = {weekIndex: weekIndex, week: week}


weekIndex は id
week は value
が見やすいと個人的には思います🤔

weekObjはすぐに配列に格納するので、以下でどうでしょうか

weeks.push({id: id, value: value})

kasai441 · 2021-07-01T02:47:18Z

app/javascript/niconico_calendar.vue

+      const lastDay = new Date(this.calendarYear, this.calendarMonth, 0)
+      return lastDay.getDate()
+    },
+    calendarSquares() {


変数名は好みですが、calendarDatesなどが明示的で良いかもしれません🤔

nullを含めた配列だったのでカレンダーのマスの意味合いでsquareにしてたんですが、dateの方がわかりやすそうですね。

kasai441 · 2021-07-01T04:43:38Z

app/javascript/niconico_calendar.vue

+              )
+                .niconico-calendar__day-label {{ date.date }}
+                .niconico-calendar__day-value
+                  img.niconico-calendar__emotion-image(


ここでdate.emotionがnullの場合エラーを吐いているようです。あえてエラーを出させないのであれば以下のコードが必要かもしれませんが、放置してもいいような気もします🤔

v-if='date.emotion'

sample: ユーザー'hajime'の最初の日報

今の仕様だとemotionがnullの日報は作れないので特に対応しなくてもいいかなと思いました。

kasai441 · 2021-07-01T05:54:09Z

app/javascript/niconico_calendar.vue

+      h2.card-header__title
+        | ニコニコカレンダー
+    .card-body(v-if='!loaded')
+      | ロード中


日報がないユーザーの場合、ずっとロード中になるので、条件を分岐して、「日報がありません」のようなメッセージを出すといいかと思います。

これも気付いていませんでした。
日報がない場合の文言を表示するようにしました。

kasai441 · 2021-07-01T06:59:40Z

すみません、一点追記です。
テストのlogin_userはvisit_with_authに差し替えとなっています。
mainでは既に反映されているのでプルリクでも書き換えておいた方が良いようです。

obregonia1 · 2021-07-04T09:16:11Z

@kasai441

カレンダーをめくってリンクから日報に遷移した後、ブラウザバックすると最新の月に戻ってしまう

この仕様も未確認でした。これは特に要件にはなかったので新たにIssueを立てておこうと思います！
指摘された点修正したので再度レビューよろしくお願いします🙇‍♂️

kasai441

変更点確認、動作確認しましたー、OKですー
$nextTickだと、可読性もよくていいですね！

obregonia1 · 2021-07-05T01:19:17Z

@kasai441
ありがとうございます！
approveされていないみたいなのでよろしくお願いします🙇‍♂️

@komagata
こちらレビューよろしくお願いします🙇‍♂️

komagata

conflictの修正お願いします〜

obregonia1 · 2021-07-05T23:49:22Z

@komagata
conflict解消したのですが、町田さんにデザイン依頼し忘れていたので先にデザインをお願いしようと思います。

@machida
app/javascript/niconico_calendar.vueのデザインよろしくお願いします🙇‍♂️

obregonia1 · 2021-07-05T23:50:24Z

app/javascript/niconico_calendar.vue

+      .calendar__head
+        .calendar__head--previous(
+          v-show='!oldestMonth()'
+          @click='previousMonth'
+        ) <
+        .calendar__head--year--month {{ calendarYear }}年{{ calendarMonth }}月
+        .calendar__head--next(
+          v-show='!newestMonth()'
+          @click='nextMonth'
+          ) >


@machida
デザイン必要な部分です。

machida · 2021-07-13T08:53:24Z

@komagata @obregonia1 デザイン入れましたー

obregonia1 · 2021-07-13T09:19:22Z

@machida
ありがとうございます！

@komagata
こちらレビューよろしくお願いします！

komagata · 2021-07-13T13:51:36Z

app/javascript/niconico_calendar.js

+import NiconicoCalendar from './niconico_calendar'
+
+document.addEventListener('DOMContentLoaded', () => {
+  const niconicoCalendar = document.getElementById('js-niconico_calendar')


15行目とselectorが2回書かれているので変数に入れて共通化すると良さそうです〜

komagata · 2021-07-13T13:54:03Z

app/javascript/niconico_calendar.vue

+
+<style scoped>
+
+</style>


不要なら削除しちゃってください〜

obregonia1 · 2021-07-13T22:34:28Z

@komagata
指摘された点修正したので再度確認お願いしますー。

komagata

確認しました、OKですー🙆‍♂️

obregonia1 · 2021-07-14T22:44:20Z

@komagata
ありがとうございます！
これでチーム開発で割り振られたIssue全て完了になりました🙌
とても勉強になりました〜✍️

komagata · 2021-07-15T05:07:03Z

@obregonia1 おおお〜、おめでとうございます！お疲れ様でした〜！！

obregonia1 added 12 commits June 25, 2021 22:17

ニコニコカレンダーを表示するために必要な情報を取得するAPIを作成

5b48557

ニコニコカレンダーのJSファイルとVueファイルを作成

e3053aa

fixtureに一ヶ月前の日報を追加

1b5e351

前月、翌月を移動できるようにページャーを実装

0615356

nullを含めたカレンダーの配列を作成

b9d967c

カレンダーを一週間毎に分けた配列を作成

6041fbe

カレンダーに表示される画像を/publicに配置した

9da9e4c

取得したcalendarWeeksAryを元にカレンダーを表示させた

8f9f6b7

ニコニコカレンダーのページャーが作動するかのテストを追加

f5e16bf

ニコニコカレンダーをVue.js化し、不要になったviewファイル、メソッドを削除

babf5d9

カレンダーの読み込み中にロード中の文字を表示させた

6235d61

使用しなくなったモデルテストを削除

b95454b

obregonia1 self-assigned this Jun 26, 2021

obregonia1 requested a review from kasai441 June 26, 2021 06:20

👮‍♀️

a252e12

kasai441 reviewed Jul 1, 2021

View reviewed changes

obregonia1 added 7 commits July 3, 2021 10:30

カレンダーの表示に使用していた定数を削除

c5d2b1a

変数名をcalendarSquaresからcalendarDatesに変更

488eb34

日報の日付をif文で判定する箇所を、メソッドを作り可読性をよくした

8c152ed

月を遷移させるときにコンポーネントを明示的に再描画した

2b85419

カレンダーを週ごとに分けた配列の変数名を変更

0017d0b

日報がない場合の文言を表示するようにした

794e24b

テストのヘルパーメソッドを変更

822778b

obregonia1 requested a review from kasai441 July 4, 2021 09:16

kasai441 reviewed Jul 5, 2021

View reviewed changes

obregonia1 requested a review from komagata July 5, 2021 01:19

kasai441 approved these changes Jul 5, 2021

View reviewed changes

komagata requested changes Jul 5, 2021

View reviewed changes

Merge branch 'main' into feature/display_caleandar_per_month

9778134

obregonia1 requested a review from machida July 5, 2021 23:41

obregonia1 commented Jul 5, 2021

View reviewed changes

komagata assigned machida Jul 6, 2021

obregonia1 removed the request for review from machida July 7, 2021 05:13

machida added 2 commits July 13, 2021 17:47

ニコニコカレンダーのデザイン修正

9044e69

矢印の色を変更

dfa9bb6

obregonia1 requested a review from komagata July 13, 2021 09:19

komagata requested changes Jul 13, 2021

View reviewed changes

obregonia1 added 2 commits July 14, 2021 07:24

使用していないstyleタグを削除

506f85a

selectorを変数に代入し共通化した

37c01f3

komagata approved these changes Jul 14, 2021

View reviewed changes

komagata merged commit 498f7cb into main Jul 14, 2021

komagata deleted the feature/display_caleandar_per_month branch July 14, 2021 14:19

github-actions bot mentioned this pull request Jul 14, 2021

Release 2021-07-15 05:18:13 #2947

Merged

19 tasks


		<style scoped>

		</style>

ニコニコカレンダーをvue.jsにし、月ごとの表示にして、ページャーを付けた #2886

ニコニコカレンダーをvue.jsにし、月ごとの表示にして、ページャーを付けた #2886

Conversation

obregonia1 commented Jun 26, 2021 • edited Loading

概要

obregonia1 commented Jun 26, 2021

kasai441 commented Jun 28, 2021

obregonia1 commented Jun 30, 2021

kasai441 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obregonia1 Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasai441 commented Jul 1, 2021 • edited Loading

obregonia1 commented Jul 4, 2021

kasai441 left a comment

Choose a reason for hiding this comment

obregonia1 commented Jul 5, 2021

komagata left a comment

Choose a reason for hiding this comment

obregonia1 commented Jul 5, 2021

Choose a reason for hiding this comment

machida commented Jul 13, 2021

obregonia1 commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obregonia1 commented Jul 13, 2021

komagata left a comment

Choose a reason for hiding this comment

obregonia1 commented Jul 14, 2021

komagata commented Jul 15, 2021

obregonia1 commented Jun 26, 2021 •

edited

Loading

obregonia1 Jul 4, 2021 •

edited

Loading

kasai441 commented Jul 1, 2021 •

edited

Loading