chore: Calendarの内部処理を整理する #5346
Conversation
…e-refactoring-Calendar-YearPicker
yagimushi
force-pushed
the
chore-refactoring-Calendar
branch
from
01bc342 to
8a18400
Compare
commit: |
yagimushi
force-pushed
the
chore-refactoring-Calendar
branch
from
8a18400 to
1c2384a
Compare
yagimushi
force-pushed
the
chore-refactoring-Calendar
branch
from
e9a39e5 to
5a054a4
Compare
| const result: Array<Array<number | null>> = [] | ||
|
|
||
| for (let weekIndex = 0; weekIndex < numOfWeek; weekIndex++) { | ||
| // 週毎の配列を形成 | ||
| const startDateInWeek = weekIndex * 7 - startDay + 1 | ||
| return Array.from({ length: 7 }).map((__, dateIndex) => { | ||
| const weekNumbers: Array<number | null> = [] | ||
|
|
||
| for (let dateIndex = 0; dateIndex < 7; dateIndex++) { | ||
| // 1週の配列を形成 | ||
| const dateNum = startDateInWeek + dateIndex | ||
|
|
||
| return dateNum > 0 && dateNum <= lastDate ? dateNum : null | ||
| }) | ||
| }) | ||
| weekNumbers[dateIndex] = dateNum > 0 && dateNum <= lastDate ? dateNum : null | ||
| } | ||
|
|
||
| result[weekIndex] = weekNumbers | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
Array.from({ length: X }) で特定の長さの配列を作ったあとmap... という思考の流れが微妙にわかりづらく、またネストしていることがそれを助長してしまっているので
- 純粋に空配列を作成
- 空配列に対してforで何回ループするのかを明示
することで配列の生成とループ回数を明確に分離しました
(一応forのほうが高速だし...)
There was a problem hiding this comment.
これ複数回同じような操作が出てきていて見通しが悪いので、rangeみたいなジェネレーター関数をutilsとかに定義してあげてもいいかもと思いました
function* range(start, end) {
for (let i = start; i < end; i++) {
yield i;
}
}There was a problem hiding this comment.
あー、↑みて思い出したんですが、Paginationコンポーネント内で外部module使ってrange変数使ってた気が...
そっちに置き換えますか
There was a problem hiding this comment.
@Qs-F 検討したんですが
weekNumbers[dateIndex] = dateNum > 0 && dateNum <= lastDate ? dateNum : null
があり、数値を常に突っ込み続けられるロジックにはできない(前後どちらかにnullが入る場合がある)のであまりrangeを使っても読みやすくならなそうです...
| const lastDate = dayjs(date).add(1, 'month').date(0).date() | ||
| const day = dayjs(date) | ||
| const startDay = day.date(1).day() | ||
| const lastDate = day.add(1, 'month').date(0).date() |
There was a problem hiding this comment.
dayjsのdateやaddメソッドは別オブジェクトを生成するため、dayjs自体の実行結果を別変数にしました。
こうすることでメソッド呼び出し、実行のコストが浮きます
| const length = Math.max(Math.min(toYear, 9999) - fromYear + 1, 0) | ||
| const result: number[] = [] | ||
|
|
||
| for (let i = 0; i < length; i++) { | ||
| result[i] = fromYear + i | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
元の処理では
- numOfYear分の長さの配列を生成
- その後nillで全部埋める
- その後nullを無視してindexを元に年を計算....
とかなり無駄な処理をしてしまっていました。
結局何回ループするのか？ が微妙に分かりづらいので
- 空配列を生成
- 空配列に対してindexを元に年を計算して詰める
ようにロジックを変更しました
| }, | ||
| }) | ||
|
|
||
| export const YearPicker: FC<Props & ElementProps> = ({ | ||
| export const YearPicker: FC<Props & ElementProps> = ({ isDisplayed, ...rest }) => | ||
| isDisplayed ? <ActualYearPicker {...rest} /> : null |
There was a problem hiding this comment.
isDisplayedではない場合でYearPickerを常に計算するようになっていたため、
他コンポーネントと合わせて、そもそも表示しない場合はロジックを実行しないようにしました。
年の選択UIはそもそも利用されない可能性も十分に有るため、既存のロジックより効率的なはずです
| const fromDay = dayjs(from) | ||
| const toDay = dayjs(to) | ||
| // HINT: dayjsのisSameは文字列でも比較可能なため、cacheが効きやすいstringにする | ||
| const nowDateStr = dayjs().startOf('date').toString() |
There was a problem hiding this comment.
nowの情報が欲しいため、memo化できません。
ただここで生成する文字列は1日単位なのでキャシュしちゃってもいいかもですが...
| const formattedFrom = useMemo(() => { | ||
| const date = getFromDate(from) | ||
| const day = dayjs(date) | ||
|
|
||
| return { | ||
| day, | ||
| date, | ||
| year: day.year(), | ||
| } | ||
| }, [from]) | ||
| const formattedTo = useMemo(() => { | ||
| const date = getToDate(to) | ||
| const day = dayjs(date) | ||
|
|
||
| return { | ||
| day, | ||
| date, | ||
| year: day.year(), | ||
| } | ||
| }, [to]) |
There was a problem hiding this comment.
from, to はほぼ固定で変わる可能性は低いため、memo化しています
またこれらをベースに計算される値も同様です
AtsushiM
requested review from
misako0927 and
hiroki0525
and removed request for
a team
| slots: { | ||
| container: | ||
| 'smarthr-ui-Calendar shr-inline-block shr-overflow-hidden shr-rounded-m shr-bg-white shr-text-black shr-shadow-layer-3 forced-colors:shr-border-shorthand forced-colors:shr-shadow-none', | ||
| header: 'smarthr-ui-Calendar-header shr-border-b-shorthand shr-flex shr-items-center shr-p-1', | ||
| yearMonth: 'smarthr-ui-Calendar-yearMonth shr-me-0.5 shr-text-base shr-font-bold', | ||
| monthButtons: 'smarthr-ui-Calendar-monthButtons shr-ms-auto shr-flex', | ||
| tableLayout: 'shr-relative', | ||
| yearSelectButton: | ||
| 'smarthr-ui-Calendar-selectingYear [&[aria-expanded="true"]_.smarthr-ui-Icon]:shr-rotate-180', |
| @@ -13,13 +15,12 @@ import React, { | |||
| import { tv } from 'tailwind-variants' | |||
|
|
|||
| import { Button } from '../Button' | |||
| import { FaCaretDownIcon, FaCaretUpIcon, FaChevronLeftIcon, FaChevronRightIcon } from '../Icon' | |||
There was a problem hiding this comment.
note: アイコンを逆さまにして表示するようにしたので必要なくなったよう
|
|
||
| const calendar = tv({ | ||
| const classNameGenerator = tv({ |
There was a problem hiding this comment.
nits: classNamesGenerator のほうが正しいかも？と思いました
There was a problem hiding this comment.
あー、これlintで一律同じような名前に揃える、という感じになっているので...
一旦このままいかせてください。
必要に応じてlintを変えます
| /** 選択可能な開始日 */ | ||
| from: Date | ||
| /** 選択可能な終了日 */ | ||
| to: Date | ||
| /** トリガのセレクトイベントを処理するハンドラ */ | ||
| onSelectDate: (e: MouseEvent, date: Date) => void | ||
| /** 選択された日付 */ | ||
| selected?: Date | null | ||
| selectedDayStr: string |
There was a problem hiding this comment.
Strだと意図が不明瞭に感じました。意図としては表示用の日付を差し込むところだと思うので、 formattedSelectedDay とかどうでしょう？
There was a problem hiding this comment.
formattedSelectedDayでstringであることがわかるかな...
ここでは型で string 嫁いているのでわかるんですが...
selectedDayText とかだとだめですかね？
(まぁここ内部コンポーネントだしそこまで細かく考えなくても良いかもですが...)
| selectedDayStr: string | ||
| from: Date | ||
| to: Date | ||
| nowDateStr: string |
There was a problem hiding this comment.
上と同じく、Strだと意図が不明瞭に感じました。意図としては表示用の日付を差し込むところだと思うので、 formattedNowDate とかどうでしょう？
| nowDateStr: string | |
| formattedNowDate: string |
| const result: Array<Array<number | null>> = [] | ||
|
|
||
| for (let weekIndex = 0; weekIndex < numOfWeek; weekIndex++) { | ||
| // 週毎の配列を形成 | ||
| const startDateInWeek = weekIndex * 7 - startDay + 1 | ||
| return Array.from({ length: 7 }).map((__, dateIndex) => { | ||
| const weekNumbers: Array<number | null> = [] | ||
|
|
||
| for (let dateIndex = 0; dateIndex < 7; dateIndex++) { | ||
| // 1週の配列を形成 | ||
| const dateNum = startDateInWeek + dateIndex | ||
|
|
||
| return dateNum > 0 && dateNum <= lastDate ? dateNum : null | ||
| }) | ||
| }) | ||
| weekNumbers[dateIndex] = dateNum > 0 && dateNum <= lastDate ? dateNum : null | ||
| } | ||
|
|
||
| result[weekIndex] = weekNumbers | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
これ複数回同じような操作が出てきていて見通しが悪いので、rangeみたいなジェネレーター関数をutilsとかに定義してあげてもいいかもと思いました
function* range(start, end) {
for (let i = start; i < end; i++) {
yield i;
}
}…e-refactoring-Calendar
関連URL
概要
変更内容
確認方法