traQ上のメッセージの投稿・編集日時に年月日を適宜省略しつつ付け足す #4338
Conversation
|
Preview (prod) → https://4338-prod.traq-preview.trapti.tech/ |
ありがとうございます！testを修正しました。
具体的には、現在日時と編集日時との関係によって日付表示が『YYYY/MM/DD』『MM/DD』『昨日』『今日』の4通りに変化するので、それぞれについて正しく日付表示が場合分けされているかを検証するように書き換えました。現在日時を考慮してテストケースを作成する必要があるので多少コードが煩雑になってしまいました（とくに『MM/DD』の表示条件に合致する日付を常に生成するために工夫が必要でした） |
|
なるほど、現在時刻を動かせるんですね…。勉強になります。確かにシンプルに書けました |
src/lib/basic/date.ts
Outdated
| let displayDate = new Date(createdAt) | ||
| if (createdAt !== updatedAt) { | ||
| displayDate = new Date(updatedAt) | ||
| } |
There was a problem hiding this comment.
imo: 3項演算子使った方がシンプルに書けそうな気がします:eyes:
There was a problem hiding this comment.
というかこれ常にnew Date(updatedAt)で良くないですか？
There was a problem hiding this comment.
確かに三項演算子は使えますね。修正前のコードの構造に気を取られて気付かなかったです。
メッセージの投稿直後に updatedAt = createdAt として定義してくれているなら単に1行
displayDate = new Date(updatedAt)でも良さそうには思えたのですが、その確証がなかったので残しました。これを採用すると createdAt を引数にとる必要がなくなります
src/lib/basic/date.ts
Outdated
| const yesterday = new Date(today) | ||
| yesterday.setDate(today.getDate() - 1) |
There was a problem hiding this comment.
imo: あんまり再代入したくない気持ちがある
const yesterday = new Date(today.getTime() - 1000 * 60 * 60 * 24)
src/lib/basic/date.ts
Outdated
| if (getFullDayString(displayDate) === getFullDayString(today)) { | ||
| return '今日' + ' ' + timeString | ||
| } | ||
| if (getFullDayString(displayDate) === getFullDayString(yesterday)) { | ||
| return '昨日' + ' ' + timeString | ||
| } | ||
| if (displayDate.getFullYear() === today.getFullYear()) { | ||
| return getDayString(displayDate) + ' ' + timeString | ||
| } else { | ||
| const updatedDate = new Date(updatedAt) | ||
| return getDateRepresentationWithoutSameDate(updatedDate, createdDate) | ||
| return getFullDayString(displayDate) + ' ' + timeString | ||
| } |
There was a problem hiding this comment.
ここはフォーマット後の文字列を比較するより素直に年月日を比較してフォーマットを決めるのが他の関数の中身を読む手間を減らせて分かりやすそうです
| ) { | ||
| return '昨日' + ' ' + timeString | ||
| } | ||
| if (displayDate.getFullYear() === today.getFullYear()) { |
There was a problem hiding this comment.
ここから上は年が同じである条件はおなじなのでネストは増えるけど条件をまとめられそうです
There was a problem hiding this comment.
文意をうまく汲み取れていないかもですが、こういうことですか…？
if (
displayDate.getFullYear() === yesterday.getFullYear() &&
displayDate.getMonth() === yesterday.getMonth() &&
displayDate.getDate() === yesterday.getDate()
) {
return '昨日' + ' ' + timeString
}
if (displayDate.getFullYear() === today.getFullYear()) {
if (
displayDate.getMonth() === today.getMonth() &&
displayDate.getDate() === today.getDate()
) {
return '今日' + ' ' + timeString
}
return getDayString(displayDate) + ' ' + timeString
}
return getFullDayString(displayDate) + ' ' + timeString大晦日のメッセージを元日に閲覧する時には『20XX/12/31』ではなく『昨日』と表示されてほしいので、年が同じである条件をまとめるならばそれより先に『昨日』の判定を書くことになります。
しかし個人的には、メッセージの投稿日時が
『今日』かどうか →『昨日』かどうか →『今年』かどうか
と順に遡りつつ判定していく構造を保った方が整理された実装に見えるような気がします…。どうでしょうか？
There was a problem hiding this comment.
↑のコードの想定でした
todayとyesterday混同しててもう少し単純なコードになると勘違いしてました🙏
それを踏まえても個人的にはtodayの比較とyesterdayの比較が交互に来るよりもyesterdayの比較→todayの比較の順に比較できる↑の実装が見やすいと感じますが、さっきの文字列で比較しないで欲しいという指摘ほど強い思いはないので任せます
There was a problem hiding this comment.
や、共通化のことだけを考えてたんですが拡張性が低くなるのでやっぱり現在に近い順に記述するままでいいです🙇🙇🙇
There was a problem hiding this comment.
了解です。レビューありがとうございます！
#3746
↑に関連していますが、当初のIssueが求める通りの実装ではありません。
表示するよう
date.ts内の関数getDisplayDateを書き換えました。開発環境ではうまくいっているように見えましたが、この変更がtraQに表示されるメッセージの日付以外に影響を与える可能性について把握し切れていないのでレビューをお願いしたいです。