-
Notifications
You must be signed in to change notification settings - Fork 40
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
traQ上のメッセージの投稿・編集日時に年月日を適宜省略しつつ付け足す #4338
Changes from 6 commits
c9ae32b
8e949db
6c40343
0fb8434
1704661
a35b1b0
292f68a
3480347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,12 +42,25 @@ export const getDateRepresentationWithoutSameDate = ( | |
} | ||
|
||
export const getDisplayDate = (createdAt: string, updatedAt: string) => { | ||
const createdDate = new Date(createdAt) | ||
if (createdAt === updatedAt) { | ||
return getTimeString(createdDate) | ||
let displayDate = new Date(createdAt) | ||
if (createdAt !== updatedAt) { | ||
displayDate = new Date(updatedAt) | ||
} | ||
const today = new Date() | ||
const timeString = getTimeString(displayDate) | ||
const yesterday = new Date(today) | ||
yesterday.setDate(today.getDate() - 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo: あんまり再代入したくない気持ちがある const yesterday = new Date(today.getTime() - 1000 * 60 * 60 * 24) |
||
|
||
if (getFullDayString(displayDate) === getFullDayString(today)) { | ||
return '今日' + ' ' + timeString | ||
} | ||
if (getFullDayString(displayDate) === getFullDayString(yesterday)) { | ||
return '昨日' + ' ' + timeString | ||
} | ||
if (displayDate.getFullYear() === today.getFullYear()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここから上は年が同じである条件はおなじなのでネストは増えるけど条件をまとめられそうです There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 文意をうまく汲み取れていないかもですが、こういうことですか…? 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ↑のコードの想定でした それを踏まえても個人的にはtodayの比較とyesterdayの比較が交互に来るよりもyesterdayの比較→todayの比較の順に比較できる↑の実装が見やすいと感じますが、さっきの文字列で比較しないで欲しいという指摘ほど強い思いはないので任せます There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. や、共通化のことだけを考えてたんですが拡張性が低くなるのでやっぱり現在に近い順に記述するままでいいです🙇🙇🙇 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 了解です。レビューありがとうございます! |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここはフォーマット後の文字列を比較するより素直に年月日を比較してフォーマットを決めるのが他の関数の中身を読む手間を減らせて分かりやすそうです |
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo: 3項演算子使った方がシンプルに書けそうな気がします:eyes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
というかこれ常に
new Date(updatedAt)
で良くないですか?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに三項演算子は使えますね。修正前のコードの構造に気を取られて気付かなかったです。
メッセージの投稿直後に
updatedAt = createdAt
として定義してくれているなら単に1行でも良さそうには思えたのですが、その確証がなかったので残しました。これを採用すると
createdAt
を引数にとる必要がなくなります